Online repl checkpoint

[knizhnik]

Checkpoint replication state not only in shutdown, but also in online checkpoint

[tristan957]

So @MMeent added this comment:

+/*
+ * NEON:  we use logical records to persist information of about slots, origins, relation map...
+ * If it is done inside shutdown checkpoint, then Postgres panics: "concurrent write-ahead log activity while database system is shutting down"
+ * So it before checkpoint REDO position is determined.
+ */

It sounds like the PR as written wouldn't account for this panic? Should we keep everything the same, but just move the call to CheckPointSnapBuild() after the if/else in CheckPointReplicationState()?

[tristan957]

Actually, after thinking about this more, it looks good. Let's get a review from Matthias too.

[tristan957]

A more descriptive commit message would also be useful for future git archaeologists.

[knizhnik]

So @MMeent added this comment:

+/*
+ * NEON:  we use logical records to persist information of about slots, origins, relation map...
+ * If it is done inside shutdown checkpoint, then Postgres panics: "concurrent write-ahead log activity while database system is shutting down"
+ * So it before checkpoint REDO position is determined.
+ */

It sounds like the PR as written wouldn't account for this panic? Should we keep everything the same, but just move the call to CheckPointSnapBuild() after the if/else in CheckPointReplicationState()?

I just restore behaviour which we have for all other versions of Postgres: pg14-16.
I have added this if in pg17 to minimize number of generated AUX files.
But right now I think that it is not correct: if we do not save slots info in online checkpoints then we can loose it in case of compute crash.

In any case, I think it is good idea to have the same code in all PG versions.

[hlinnaka]

This is pretty confusing. Even before this PR, but this isn't helping..

• The function is called "CheckPointReplicationState", but CheckPointRelationMap() has nothing to do with replication.
• CheckPointRelationMap() and CheckPointReplicationOrigin() don't write any WAL records, so they don't need this special treatment. They can be called from CheckPointGuts() just like in upstream
• In a shutdown checkpoint, all these functions are now being called twice, once in the PreCheckPointGuts() stage and again in CheckPointGuts(). Seems unnecessary, but also wrong; won't those functions PANIC again, when the try to write the WAL records?

[knizhnik]

Yes, CheckPointRelationMap() has not relation to replication.
But it is called before CheckPointReplicationSlots and CheckPointLogicalRewriteHeap in CheclpointGuts this his why I thought that later ones can somehow depends on it and it is better to reserve order.

CheckPointReplicationOrigin() writes "pg_logical/replorigin_checkpoint" file. But we do not persist it using AUX mechanism. Actually I don't remember why. May be just because it is not needed because origins are wallowed in any case.

It is real bug that CheckPointReplicationState is called twice - I forget to copy correspondent check (which is done in pg 14/15/16). I will fix it.

[knizhnik]

I committed fix of double call of CheckPointReplicationState.
So now pg17 is ding exactly the same as all there versions.

Do you think that we should include in this PR all other changes: move CheckPointReplicationOrigin() and CheckPointRelationMap() out of CheckPointReplicationState?

I agree with you that they are not writing WAL and so there is no need to call them here.
But as I wrote above, I prefer to reserve order of calling this functions in CheckpointGuts and CheckPointReplicationOrigin() definitely is related to replication state, although in not walloging written file.

I prefer to leave it is as it is now or create separate PR for it, because it should affect all other Postgres versions.

[hlinnaka]

CheckPointReplicationOrigin() writes "pg_logical/replorigin_checkpoint" file. But we do not persist it using AUX mechanism. Actually I don't remember why. May be just because it is not needed because origins are wallowed in any case.

Related: neondatabase/neon#8620

[hlinnaka]

I committed fix of double call of CheckPointReplicationState. So now pg17 is ding exactly the same as all there versions.

Do you think that we should include in this PR all other changes: move CheckPointReplicationOrigin() and CheckPointRelationMap() out of CheckPointReplicationState?

I agree with you that they are not writing WAL and so there is no need to call them here. But as I wrote above, I prefer to reserve order of calling this functions in CheckpointGuts and CheckPointReplicationOrigin() definitely is related to replication state, although in not walloging written file.

I prefer to leave it is as it is now or create separate PR for it, because it should affect all other Postgres versions.

Ok, let's commit this, to bring all the Postgres versions to the same state, and open a separate PR for the other cleanup.