Retry post-copy VACUUM ANALYZE on transient target connection loss - #48
Merged
Conversation
A momentary connection loss (e.g. an SSL EOF) during the optional post-copy VACUUM ANALYZE step was treated as fatal, failing the worker even though the data copy itself succeeded. - Mark the connection PG_CONNECTION_BAD on async teardown so callers can tell a lost connection from a genuine SQL error via pgsql->status. - Guard pgsql_execute_log_error against a NULL connection so the misleading "connection pointer is NULL" line no longer masks the real error; still log the SQL query/params context. - Retry VACUUM ANALYZE up to 3 times on connection loss, each attempt on a fresh connection. Real SQL errors are not retried; the step stays fatal once retries are exhausted.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
During a migration, the post-copy VACUUM ANALYZE step failed on a single table when the target's SSL connection dropped mid-query:
A momentary network blip during the optional, post-copy VACUUM ANALYZE step was treated as a fatal, non-retryable error, and the real cause (SSL EOF) was buried under a misleading "connection pointer is NULL" line. The data copy itself succeeded; only the optimization step tripped, yet the whole worker exited non-zero.
Changes
pgsql_stream_log_error: setpgsql->status = PG_CONNECTION_BADbefore the teardownpgsql_finish. This is the single teardown path for all threepgsql_fetch_resultserror branches (invalid socket, select failed,PQconsumeInput == 0), so every mid-query connection loss becomes observable to callers viapgsql->status.pgsql_execute_log_error: guard the libpq message/PID block whenconnection == NULL.PQerrorMessage(NULL)returns the literal "connection pointer is NULL"; the real error was already logged upstream. The useful SQL query/params context is still logged, with a backend PID of 0. Mirrors the existing guard inlog_connection_error.vacuum_analyze_table_by_oid: retry VACUUM ANALYZE up to 3 times on a lost/broken connection. Each attempt uses a fresh connection; the interactive open-retry policy (60s) absorbs the reconnect wait. OnlyPG_CONNECTION_BADis retried — genuine SQL errors (lock timeout, undefined table, etc.) leave the status OK and are not retried. The step stays fatal once retries are exhausted.Testing (PostgreSQL 18)
make buildmake tests/pagilacitus_indent --checkmake tests(full suite)A deterministic mid-VACUUM SSL EOF is not practical to inject in the test harness, so the retry path was verified by code trace: (a)
dst.statusisPG_CONNECTION_BADafter an async teardown, (b) a real non-class-08 SQL error leaves status OK and is not retried, and (c) the "connection pointer is NULL" line no longer appears.