Rework subscription-related code for psql and pg_dump
Hi all,
While looking at some of the code related to subscriptions in psql,
coming down to make LOGICALREP_STREAM_* available for client code,
I've also noticed quite a few inconsistencies and mistakes with how
pg_subscription is handled in pg_dump, that have accumulated over the
years as of what looks like set of copy-pastos for most of them:
- pg_dump.c includes pg_subscription.h and not pg_subscription_d.h.
This is a mistake as the former should only be included in the backend
code.
- SubscriptionInfo has accumulated dust over the years, using
declarations types that don't map with its catalog. To keep it short
here, all the fields use (char *) leading to more DIY logic in the
code (see two_phase_disabled[]), while most of the fields are booleans
or char values. Switching to char values allows direct comparisons
with the contents of pg_subscription_d.h, leading to more consistent
code.
- Inconsistent position of fields between SubscriptionInfo and the
catalog pg_subscription.
EXPOSE_TO_CLIENT_CODE has been added to pg_subscription.h so as values
for substream, twophasestate and origin can be used directly in psql
and pg_dump, switching these to use pg_subscription_d.h as this is
client code.
While all that addressed, I am finishing with the patch attached.
Thoughts or comments?
--
Michael
Attachments:
0001-Refactor-subscription-code-for-psql-and-pg_dump.patchtext/x-diff; charset=us-asciiDownload+58-57
Dear Michael,
Thanks for working on this.
It was not good to follow existing codes without confirmation :-(.
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.
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.
Thought?
Best regards,
Hayato Kuroda
FUJITSU LIMITED
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
On Sat, Nov 30, 2024 at 12:54:19PM +0900, Michael Paquier wrote:
- 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.
I've looked at that again and let this as-is, then applied all this
cleanup.
--
Michael