Set GUC values in pgsql_create_logical_replication_slot()

[igo-git]

In the pgsql_create_logical_replication_slot() function, GUC values are not set for the connection, holding a snapshot.

Therefore, if idle_in_transaction_session_timeout is set on the source, the session may be terminated due timeout and the snapshot may be lost.

[dimitri]

Thanks for the fix — this is a real bug and the approach is on the right track.

The problem with the current implementation

pgsql.c is the only file in this binary that does not include copydb.h, and that appears intentional: it's the lowest-level database-connection module, and copydb.h pulls in a large slice of the higher-level orchestration layer (cli_common.h, filtering.h, schema.h, summary.h, …). Adding that include creates an architectural inversion that I'd like to avoid.

Suggested alternative

The cleanest fix is to add a GUC *settings parameter to pgsql_create_logical_replication_slot() and let the caller pass the right array. The caller in snapshot.c already has the connection (and therefore the PG version) in scope, so it can do exactly what copydb_export_snapshot() already does:

/* snapshot.c — after pgsql_init_stream */
if (!pgsql_server_version(&stream->pgsql)) { return false; }

GUC *settings =
    stream->pgsql.pgversion_num < 90600 ? srcSettings95 : srcSettings;

if (!pgsql_create_logical_replication_slot(stream, slot, settings))
    ...

/* pgsql.c — function signature */
bool pgsql_create_logical_replication_slot(LogicalStreamClient *client,
                                           ReplicationSlot *slot,
                                           GUC *settings)

Inside pgsql_create_logical_replication_slot, the body stays as you wrote it — just replace the local settings derivation with the passed-in parameter, and drop the #include "copydb.h".

One question worth confirming

The GUCs are applied via SET on a replication=database connection. PostgreSQL's walsender does process SQL queries on such connections, so this should work — but it would be good to confirm with a quick test against a source that actually has idle_in_transaction_session_timeout set to a short value (e.g. 1s). Did you test in that configuration?

[igo-git]

The problem with the current implementation

Thank you for reviewing the patch. I've fixed it according to your recommendations.

The GUCs are applied via SET on a replication=database connection. PostgreSQL's walsender does process SQL queries on such connections, so this should work — but it would be good to confirm with a quick test against a source that actually has idle_in_transaction_session_timeout set to a short value (e.g. 1s). Did you test in that configuration?

I tested the fix as you suggested, and GUC idle_in_transaction_session_timeout is indeed set in the session, and the snapshot is not lost. Furthermore, we have repeatedly migrated a large production database from a server with a non-zero idle_in_transaction_session_timeout set, and the fix worked correctly.