Re: [PATCH] Fix sequence value may rollback after CREATE DATABASE TEMPLATE with WAL_LOG strategy

Hi, thank you for your review and comments!

> 1/ What happens if we reset the log_cnt for some sequences and
> the CREATE DATABASE then fails (say, while copying other
> remaining relations' data pages)? We would have written reset
> WAL records, but the CREATE DATABASE failed - and those WAL
> records now not only need to be replayed for crash recovery but
> also flow through to standbys and logical replication clients.

If the CREATE DATABASE fails in the middle of process, all
relations including sequences are properly cleaned up with
createdb_failure_callback. In my patch, WAL records are output
for sequences additionally, but note that WAL_LOG strategy already
writes WAL for every copied block and the sequence reset WAL is
just one more record in the same category.
Also, resetting the log_cnt is performed on the newly copied
database. Therefore, even if CREATE DATABASE fails in the middle
of process, the sequence of the source database is not affected.
And, in logical replication, sequences are not replicated. Even in
PostgreSQL 19, instead of decoding WAL, ALTER SUBSCRIPTION
REFRESH connects to publisher and gets the value and replicates
to subscribers. Therefore, I think any special considerations
for logical replication are not necessary.

https://www.postgresql.org/docs/19/logical-replication-sequences.html
https://github.com/postgres/postgres/blob/master/src/include/access/rmgrlist.h

> 2/ Don't we need to gate the WAL logging in
> ResetSequenceLogCnt with RelationNeedsWAL(rel)?

The current patch uses "if (permanent)". This is similar to
RelationNeedsWAL(rel).
The "permanent" field of CreateDBRelInfo contains the information
about whether the relation is permanent. ResetSequenceLogCnt uses
the information in this field to decide whether to write a WAL
record. This logic is from RelationCopyStorageUsingBuffer.

> 3/ WAL logging in ResetSequenceLogCnt and other sequence.c look
> mostly similar - try to dedup it with a similar one there?

Certainly, the existing code has similar WAL logging processing
in the three functions of sequence.c.
I think it is possible to dedup the WAL logging process and
create a shared function, and to call that new function from
three existing functions and ResetSequenceLogCnt function.
However, I think this is a bit of refactoring. With this bug
fix, should I refactor as well? (I did not include it in this
reply now, but I can provide the refactored patch.)

> 4/ Why not move ResetSequenceLogCnt closer to its friends in
> sequence.c?

I certainly thought it would be better to move it to sequence.c,
so I fixed it.

> 5/ Do we need any special handling for temporary sequences?

It's unnecessary. Temporary relations do not need to be
replicated to the standby with streaming replication.
Also, temporary relations are not appended to the rlocatorlist.
This is determined by ScanSourceDatabasePgClassTuple.

===
if (classForm->reltablespace == GLOBALTABLESPACE_OID ||
!RELKIND_HAS_STORAGE(classForm->relkind) ||
classForm->relpersistence == RELPERSISTENCE_TEMP)
return NULL;
===

> 6/ Instead of a new file, why not add the test case to existing
> src/test/recovery/t/034_create_database.pl?

Adding to the existing test file makes sense for me too. Updated.

I attached the updated v2 patch set.

Regards,