Re: Logical Replication of sequences

Hi Vignesh,

This is mostly a repeat of my previous mail from a while ago [1] but
includes some corrections, answers, and more examples. I'm going to
try to persuade one last time because the current patch is becoming
stable, so I wanted to revisit this syntax proposal before it gets too
late to change anything.

If there is some problem with the proposed idea please let me know
because I can see only the advantages and no disadvantages of doing it
this way.

~~~

The current patchset offers two forms of subscription refresh:
1. ALTER SUBSCRIPTION name REFRESH PUBLICATION [ WITH ( refresh_option
[= value] [, ... ] ) ]
2. ALTER SUBSCRIPTION name REFRESH PUBLICATION SEQUENCES

Since 'copy_data' is the only supported refresh_option, really it is more like:
1. ALTER SUBSCRIPTION name REFRESH PUBLICATION [ WITH ( copy_data [=
true|false] ) ]
2. ALTER SUBSCRIPTION name REFRESH PUBLICATION SEQUENCES

~~~

I proposed previously that instead of having 2 commands for refreshing
subscriptions we should have a single refresh command:

ALTER SUBSCRIPTION name REFRESH PUBLICATION [TABLES|SEQUENCES] [ WITH
( copy_data [= true|false] ) ]

Why?

- IMO it is less confusing than having 2 commands that both refresh
sequences in slightly different ways

- It is more flexible because apart from refreshing everything, a user
can choose to refresh only tables or only sequences if desired; IMO
more flexibility is always good.

- There is no loss of functionality from the current implementation
AFAICT. You can still say "ALTER SUBSCRIPTION sub REFRESH PUBLICATION
SEQUENCES" exactly the same as the patchset allows.

- The implementation code will become simpler. For example, the
current implementation of AlterSubscription_refresh(...) includes the
(hacky?) 'resync_all_sequences' parameter and has an overcomplicated
relationship with other parameters as demonstrated by the assertions
below. IMO using the prosed syntax means this coding will become not
only simpler, but shorter too.
+ /* resync_all_sequences cannot be specified with refresh_tables */
+ Assert(!(resync_all_sequences && refresh_tables));
+
+ /* resync_all_sequences cannot be specified with copy_data as false */
+ Assert(!(resync_all_sequences && !copy_data));

~~~

So, to continue this proposal, let the meaning of 'copy_data' for
SEQUENCES be as follows:

- when copy_data == false: it means don't copy data (i.e. don't
synchronize anything). Add/remove sequences from pg_subscriber_rel as
needed.

- when copy_data == true: it means to copy data (i.e. synchronize) for
all sequences. Add/remove sequences from pg_subscriber_rel as needed)

~~~

EXAMPLES using the proposed syntax:

Refreshing TABLES only...

ex1.
ALTER SUBSCRIPTION sub REFRESH PUBLICATION TABLES WITH (copy_data = false)
- same as PG17 functionality for "ALTER SUBSCRIPTION sub REFRESH
PUBLICATION WITH (copy_data = false)"

ex2.
ALTER SUBSCRIPTION sub REFRESH PUBLICATION TABLES WITH (copy_data = true)
- same as PG17 functionality for "ALTER SUBSCRIPTION sub REFRESH
PUBLICATION WITH (copy_data = true)"

ex3. (using default copy_data)
ALTER SUBSCRIPTION sub REFRESH PUBLICATION TABLES
- same as ex2.

~

Refreshing SEQUENCES only...

ex4.
ALTER SUBSCRIPTION sub REFRESH PUBLICATION SEQUENCES WITH (copy data = false)
- this adds/removes only sequences to pg_subscription_rel but doesn't
update the sequence values

ex5.
ALTER SUBSCRIPTION sub REFRESH PUBLICATION SEQUENCES WITH (copy data = true)
- this adds/removes only sequences to pg_subscription_rel and also
updates (synchronizes) all sequence values.
- same functionality as "ALTER SUBSCRIPTION sub REFRESH PUBLICATION
SEQUENCES" in your current patchset

ex6. (using default copy_data)
ALTER SUBSCRIPTION sub REFRESH PUBLICATION SEQUENCES
- same as ex5.
- note, that this command has the same syntax and functionality as the
current patchset

~~~

When no object_type is specified it has intuitive meaning to refresh
both TABLES and SEQUENCES...

ex7.
ALTER SUBSCRIPTION sub REFRESH PUBLICATION WITH (copy_data = false)
- For tables, it is the same as the PG17 functionality
- For sequences it includes the same behaviour of ex4.

ex8.
ALTER SUBSCRIPTION sub REFRESH PUBLICATION WITH (copy_data = true)
- For tables, it is the same as the PG17 functionality
- For sequences it includes the same behaviour of ex5.
- There is one subtle difference from the current patchset because
this proposal will synchronize *all* sequences instead of only new
ones. But, this is a good thing. The current documentation is
complicated by having to explain the differences between REFRESH
PUBLICATION and REFRESH PUBLICATION SEQUENCES. The current patchset
also raises questions like how the user chooses whether to use
"REFRESH PUBLICATION SEQUENCES" versus "REFRESH PUBLICATION WITH
(copy_data=true)". OTHO, the proposed syntax eliminates ambiguity.

ex9. (using default copy_data)
ALTER SUBSCRIPTION sub REFRESH PUBLICATION
- same as ex8

======
[1] https://www.postgresql.org/message-id/CAHut%2BPuFH1OCj-P1UKoRQE2X4-0zMG%2BN1V7jdn%3DtOQV4RNbAbw%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia