Re: Rework subscription-related code for psql and pg_dump

On Fri, Nov 29, 2024 at 05:42:13AM +0000, Hayato Kuroda (Fujitsu) wrote:
> It was not good to follow existing codes without confirmation :-(.

No problem.

> I grepped files in bin/ and could not find lines which includes catalog/pg_xxx.h files.
> (One exception is pg_control.h. It is not a catalog-header but has the same prefix.)
> The patch basically LGTM.

Thanks for double-checking.

Note also the tweak on top of pg_resetwal.c with its "#define FRONTEND
1" while declaring postgres.h. This is also one of the historical fun
things with the frontend code. Perhaps we'll have a cleaner split at
some point in the future.

> One comment...
> In describeSubscriptions(), only the streaming option is represented as {off, on, parallel},
> whereas twophase option is {d, p, e}.
> I feel it is bit strange so we can fix to show like {disabled,
> pending, enabled} by the same approach.

- if (strcmp(subinfo->subtwophasestate, two_phase_disabled) != 0)
+ if (subinfo->subtwophasestate != LOGICALREP_TWOPHASE_STATE_DISABLED)
appendPQExpBufferStr(query, ", two_phase = on");

I'm not feeling strongly either way. The code intentionally wants to
set two_phase to "on" if the catalog state is "pending" or "on", so
sticking with the current assumption of the code and keeping it as
proposed in the patch is fine, IMO.
--
Michael