Re: Resetting recovery target parameters in pg_createsubscriber

On Mon, Jan 05, 2026 at 03:33:17PM +0900, Michael Paquier wrote:
> It seems roughly OK, so I have put my hands on it.  A couple of notes
> regarding the things done in the updated version attached:
> - Addition of two tests to check pg_createsubscriber.conf.disabled in
> the data folders of node S and K.
> - More description in the tests.
> - The "dry run mode" node has disappeared from the recovery parameter
> StringInfo, so added it back at the top of the parameters generated.
> - Missing a newline after the include_if_exists.
> - dirable_rename() logs already something on failure, I see no need
> for an extra warning to say the same.  Adding the warning telling that
> a manual intervention may be required is good, though.
> - Let's group the document change with the main patch.
> - More stylistic changes, comments and code.
> - The new test fails if we undo the changes in pg_createsubscriber.c,
> as we'd want, in the shape of node K FATAL-ing due to an incorrect
> recovery configuration fed to the node.

After a second round of review, there was still an issue here: the LSN
returned by create_logical_replication_slot() could be NULL in
dry-run mode, and the code sets recovery_target_lsn to
InvalidXLogRecPtr in the recovery configuration printed out, meaning a
NULL pointer dereference.  I have switched the patch to do what
happens on HEAD: InvalidXLogRecPtr for the dry-run mode.  That also
leads to less diffs.  And done.
--
Michael