Hi, On 2021-01-25 12:00:08 -0800, Andres Freund wrote: > > > /* > > > * For backward compatibility reasons this has to be stored in the > > > wrongly > > > * named field. Will be fixed in next major version. > > > */ > > > return builder->was_running.was_xmax; > > > > We could fix that now... Andres, what did you have in mind for a proper > > name? > > next_phase_at seems like it'd do the trick?
See attached patch...
>From 5c7d735d71beae1f6e6616f1a160543fae3ac91b Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Mon, 25 Jan 2021 12:47:04 -0800 Subject: [PATCH] Remove backwards compat ugliness in snapbuild.c. In 955a684e040 we fixed a bug in initial snapshot creation. In the course of which several members of struct SnapBuild were obsoleted. As SnapBuild is serialized to disk we couldn't change the memory layout. Unfortunately I subsequently forgot about removing the backward compat gunk, but luckily Heikki just reminded me. This commit bumps SNAPBUILD_VERSION, therefore breaking existing slots (which is fine in a major release). Author: Andres Freund Reminded-By: Heikki Linnakangas <hlinn...@iki.fi> Discussion: https://postgr.es/m/c94be044-818f-15e3-1ad3-7a7ae2dfe...@iki.fi --- src/backend/replication/logical/snapbuild.c | 103 ++++---------------- 1 file changed, 18 insertions(+), 85 deletions(-) diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c index e903e561afc..752cf2d7dbc 100644 --- a/src/backend/replication/logical/snapbuild.c +++ b/src/backend/replication/logical/snapbuild.c @@ -189,24 +189,11 @@ struct SnapBuild ReorderBuffer *reorder; /* - * Outdated: This struct isn't used for its original purpose anymore, but - * can't be removed / changed in a minor version, because it's stored - * on-disk. + * TransactionId at which the next phase of initial snapshot building will + * happen. InvalidTransactionId if not known (i.e. SNAPBUILD_START), or + * when no next phase necessary (SNAPBUILD_CONSISTENT). */ - struct - { - /* - * NB: This field is misused, until a major version can break on-disk - * compatibility. See SnapBuildNextPhaseAt() / - * SnapBuildStartNextPhaseAt(). - */ - TransactionId was_xmin; - TransactionId was_xmax; - - size_t was_xcnt; /* number of used xip entries */ - size_t was_xcnt_space; /* allocated size of xip */ - TransactionId *was_xip; /* running xacts array, xidComparator-sorted */ - } was_running; + TransactionId next_phase_at; /* * Array of transactions which could have catalog changes that committed @@ -272,34 +259,6 @@ static void SnapBuildWaitSnapshot(xl_running_xacts *running, TransactionId cutof static void SnapBuildSerialize(SnapBuild *builder, XLogRecPtr lsn); static bool SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn); -/* - * Return TransactionId after which the next phase of initial snapshot - * building will happen. - */ -static inline TransactionId -SnapBuildNextPhaseAt(SnapBuild *builder) -{ - /* - * For backward compatibility reasons this has to be stored in the wrongly - * named field. Will be fixed in next major version. - */ - return builder->was_running.was_xmax; -} - -/* - * Set TransactionId after which the next phase of initial snapshot building - * will happen. - */ -static inline void -SnapBuildStartNextPhaseAt(SnapBuild *builder, TransactionId at) -{ - /* - * For backward compatibility reasons this has to be stored in the wrongly - * named field. Will be fixed in next major version. - */ - builder->was_running.was_xmax = at; -} - /* * Allocate a new snapshot builder. * @@ -728,7 +687,7 @@ SnapBuildProcessChange(SnapBuild *builder, TransactionId xid, XLogRecPtr lsn) * we got into the SNAPBUILD_FULL_SNAPSHOT state. */ if (builder->state < SNAPBUILD_CONSISTENT && - TransactionIdPrecedes(xid, SnapBuildNextPhaseAt(builder))) + TransactionIdPrecedes(xid, builder->next_phase_at)) return false; /* @@ -945,7 +904,7 @@ SnapBuildCommitTxn(SnapBuild *builder, XLogRecPtr lsn, TransactionId xid, */ if (builder->state == SNAPBUILD_START || (builder->state == SNAPBUILD_BUILDING_SNAPSHOT && - TransactionIdPrecedes(xid, SnapBuildNextPhaseAt(builder)))) + TransactionIdPrecedes(xid, builder->next_phase_at))) { /* ensure that only commits after this are getting replayed */ if (builder->start_decoding_at <= lsn) @@ -1267,7 +1226,7 @@ SnapBuildFindSnapshot(SnapBuild *builder, XLogRecPtr lsn, xl_running_xacts *runn Assert(TransactionIdIsNormal(builder->xmax)); builder->state = SNAPBUILD_CONSISTENT; - SnapBuildStartNextPhaseAt(builder, InvalidTransactionId); + builder->next_phase_at = InvalidTransactionId; ereport(LOG, (errmsg("logical decoding found consistent point at %X/%X", @@ -1299,7 +1258,7 @@ SnapBuildFindSnapshot(SnapBuild *builder, XLogRecPtr lsn, xl_running_xacts *runn else if (builder->state == SNAPBUILD_START) { builder->state = SNAPBUILD_BUILDING_SNAPSHOT; - SnapBuildStartNextPhaseAt(builder, running->nextXid); + builder->next_phase_at = running->nextXid; /* * Start with an xmin/xmax that's correct for future, when all the @@ -1331,11 +1290,11 @@ SnapBuildFindSnapshot(SnapBuild *builder, XLogRecPtr lsn, xl_running_xacts *runn * be decoded. Switch to FULL_SNAPSHOT. */ else if (builder->state == SNAPBUILD_BUILDING_SNAPSHOT && - TransactionIdPrecedesOrEquals(SnapBuildNextPhaseAt(builder), + TransactionIdPrecedesOrEquals(builder->next_phase_at, running->oldestRunningXid)) { builder->state = SNAPBUILD_FULL_SNAPSHOT; - SnapBuildStartNextPhaseAt(builder, running->nextXid); + builder->next_phase_at = running->nextXid; ereport(LOG, (errmsg("logical decoding found initial consistent point at %X/%X", @@ -1356,11 +1315,11 @@ SnapBuildFindSnapshot(SnapBuild *builder, XLogRecPtr lsn, xl_running_xacts *runn * collected. Switch to CONSISTENT. */ else if (builder->state == SNAPBUILD_FULL_SNAPSHOT && - TransactionIdPrecedesOrEquals(SnapBuildNextPhaseAt(builder), + TransactionIdPrecedesOrEquals(builder->next_phase_at, running->oldestRunningXid)) { builder->state = SNAPBUILD_CONSISTENT; - SnapBuildStartNextPhaseAt(builder, InvalidTransactionId); + builder->next_phase_at = InvalidTransactionId; ereport(LOG, (errmsg("logical decoding found consistent point at %X/%X", @@ -1463,7 +1422,7 @@ typedef struct SnapBuildOnDisk offsetof(SnapBuildOnDisk, version) #define SNAPBUILD_MAGIC 0x51A1E001 -#define SNAPBUILD_VERSION 2 +#define SNAPBUILD_VERSION 3 /* * Store/Load a snapshot from disk, depending on the snapshot builder's state. @@ -1508,6 +1467,9 @@ SnapBuildSerialize(SnapBuild *builder, XLogRecPtr lsn) if (builder->state < SNAPBUILD_CONSISTENT) return; + /* consistent snapshots have no next phase */ + Assert(builder->next_phase_at == InvalidTransactionId); + /* * We identify snapshots by the LSN they are valid for. We don't need to * include timelines in the name as each LSN maps to exactly one timeline @@ -1596,9 +1558,6 @@ SnapBuildSerialize(SnapBuild *builder, XLogRecPtr lsn) &ondisk->builder, sizeof(SnapBuild)); - /* there shouldn't be any running xacts */ - Assert(builder->was_running.was_xcnt == 0); - /* copy committed xacts */ sz = sizeof(TransactionId) * builder->committed.xcnt; memcpy(ondisk_c, builder->committed.xip, sz); @@ -1801,34 +1760,6 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn) } COMP_CRC32C(checksum, &ondisk.builder, sizeof(SnapBuild)); - /* restore running xacts (dead, but kept for backward compat) */ - sz = sizeof(TransactionId) * ondisk.builder.was_running.was_xcnt_space; - ondisk.builder.was_running.was_xip = - MemoryContextAllocZero(builder->context, sz); - pgstat_report_wait_start(WAIT_EVENT_SNAPBUILD_READ); - readBytes = read(fd, ondisk.builder.was_running.was_xip, sz); - pgstat_report_wait_end(); - if (readBytes != sz) - { - int save_errno = errno; - - CloseTransientFile(fd); - - if (readBytes < 0) - { - errno = save_errno; - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not read file \"%s\": %m", path))); - } - else - ereport(ERROR, - (errcode(ERRCODE_DATA_CORRUPTED), - errmsg("could not read file \"%s\": read %d of %zu", - path, readBytes, sz))); - } - COMP_CRC32C(checksum, ondisk.builder.was_running.was_xip, sz); - /* restore committed xacts information */ sz = sizeof(TransactionId) * ondisk.builder.committed.xcnt; ondisk.builder.committed.xip = MemoryContextAllocZero(builder->context, sz); @@ -1890,6 +1821,8 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn) if (TransactionIdPrecedes(ondisk.builder.xmin, builder->initial_xmin_horizon)) goto snapshot_not_interesting; + /* consistent snapshots have no next phase */ + Assert(ondisk.builder.next_phase_at == InvalidTransactionId); /* ok, we think the snapshot is sensible, copy over everything important */ builder->xmin = ondisk.builder.xmin; -- 2.29.2.540.g3cf59784d4
