Re: Logical decoding on standby

On 29 March 2017 at 08:11, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> On 29 March 2017 at 08:01, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
>
>> I just notice that I failed to remove the docs changes regarding
>> dropping slots becoming db-specific, so I'll post a follow-up for that
>> in a sec.
>
> Attached.

... and here's the next in the patch series. Both this and the
immediately prior minor patch fix-drop-slot-docs.patch are pending
now.

Notable changes in this patch since review:

* Split oldestCatalogXmin tracking into separate patch

* Critically, fix use of procArray->replication_slot_catalog_xmin in
GetSnapshotData's setting of RecentGlobalXmin and RecentGlobalDataXmin
so it instead uses ShmemVariableCache->oldestCatalogXmin . This
could've led to tuples newer than oldestCatalogXmin being removed.

* Memory barrier in UpdateOldestCatalogXmin and SetOldestCatalogXmin.
It still does a pre-check before deciding if it needs to take
ProcArrayLock, recheck, and advance, since we don't want to
unnecessarily contest ProcArrayLock.

* Remove unnecessary volatile usage (retained in
UpdateOldestCatalogXmin due to barrier)

* Remove unnecessary test for XLogInsertAllowed() in XactLogCatalogXminUpdate

* EnsureActiveLogicalSlotValid(void) - add (void)

* pgidented changes in this diff; have left unrelated changes alone

Re:

> what does
>
> + TransactionId oldestCatalogXmin; /* oldest xid where complete catalog state
> + * is guaranteed to still exist */
>
> mean? I complained about the overall justification in the commit
> already, but looking at this commit alone, the justification for this
> part of the change is quite hard to understand.

The patch now contains

TransactionId oldestCatalogXmin; /* oldest xid it is guaranteed to be safe
* to create a historic snapshot for; see
* also
* procArray->replication_slot_catalog_xmin
* */

which I think is an improvement.

I've also sought to explain the purpose of this change better with

/*
* If necessary, copy the current catalog_xmin needed by replication slots to
* the effective catalog_xmin used for dead tuple removal and write a WAL
* record recording the change.
*
* This allows standbys to know the oldest xid for which it is safe to create
* a historic snapshot for logical decoding. VACUUM or other cleanup may have
* removed catalog tuple versions needed to correctly decode transactions older
* than this threshold. Standbys can use this information to cancel conflicting
* decoding sessions and invalidate slots that need discarded information.
*
* (We can't use the transaction IDs in WAL records emitted by VACUUM etc for
* this, since they don't identify the relation as a catalog or not. Nor can a
* standby look up the relcache to get the Relation for the affected
* relfilenode to check if it is a catalog. The standby would also have no way
* to know the oldest safe position at startup if it wasn't in the control
* file.)
*/
void
UpdateOldestCatalogXmin(void)
{
...

Does that help?

(Sidenote for later: ResolveRecoveryConflictWithLogicalDecoding will
need a read barrier too, when the next patch adds it.)

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services