Hi Michael and Alexander,
Thank you both for your help! I really appreciate it.
As a newcomer here, I might make some mistakes, but I hope with your guidance I can avoid making them in the future =)
> Yeah, my concern was about the duplication of the list. As long as a
> fix does not do any of that, I'm OK. Sorry if my idea of a list of
> parameters felt misguided if we make recovery_gen.c smarter with the
> handling of the on-disk files.
I got your concern about avoiding duplication. I thought that defining all parameters explicitly in the file header would lead to clearer and nicer code, which is why I left it that way (even without duplicating). But now I agree with Alexander's point about keeping the changes minimal.
> This patch also means that pg_createsubscriber is written so as the
> contents added to recovery.conf/postgresql.auto.conf by
> setup_recovery() are never reset if there is a failure in-flight. Is
> that OK or should we also have an exit callback to do the cleanup work
> in such cases?
It's a good idea to add an exit callback. Additionally, I'd like to propose adding a pre-flight check at the start. This check would look for any existing recovery configuration that might be an artifact from a previous aborted run and warn the user or handle it appropriately. What do you think about implementing both the exit callback and the pre-flight check?
> pg_createsubscriber includes a
> "How it works" section that explains how the tool works, including the
> part about the recovery parameters.
I looked through the `pg_createsubscriber.c` file but wasn't able to locate a "How it works" section. Could you please point me to the specific file or line number you are referring to? Or do you mean all the descriptive comments? For context, I'm currently working on the version where my patch is being tested in CI.
I will work on improving the code and will also add the documentation notes that Michael has pointed out ASAP.
Best regards,
Alyona Vinter
