Re: Skipping logical replication transactions on subscriber side

On Fri, Jan 21, 2022 at 10:10 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Fri, Jan 21, 2022 at 1:20 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > What do we want to indicate by [, ... ]? To me, it appears like
> > multiple options but that is not what we support currently.
>
> You're right. It's an oversight.
>

I have fixed this and a few other things in the attached patch.
1.
The newly added column needs to be updated in the following statement:
-- All columns of pg_subscription except subconninfo are publicly readable.
REVOKE ALL ON pg_subscription FROM public;
GRANT SELECT (oid, subdbid, subname, subowner, subenabled, subbinary,
substream, subtwophasestate, subslotname, subsynccommit,
subpublications)
ON pg_subscription TO public;

2.
+stop_skipping_changes(bool clear_subskipxid, XLogRecPtr origin_lsn,
+ TimestampTz origin_timestamp)
+{
+ Assert(is_skipping_changes());
+
+ ereport(LOG,
+ (errmsg("done skipping logical replication transaction %u",
+ skip_xid)));

Isn't it better to move this LOG at the end of this function? Because
clear* functions can give an error, so it is better to move it after
that. I have done that in the attached.

3.
+-- fail - must be superuser
+SET SESSION AUTHORIZATION 'regress_subscription_user2';
+ALTER SUBSCRIPTION regress_testsub SKIP (xid = 100);
+ERROR: must be owner of subscription regress_testsub

This test doesn't seem to be right. You want to get the error for the
superuser but the error is for the owner. I have changed this test to
do what it intends to do.

Apart from this, I have changed a few comments and ran pgindent. Do
let me know what you think of the changes?

Few things that I think we can improve in 028_skip_xact.pl are as follows:

After CREATE SUBSCRIPTION, wait for initial sync to be over and
two_phase state to be enabled. Please see 021_twophase. For the
streaming case, we might be able to ensure streaming even with lesser
data. Can you please try that?

--
With Regards,
Amit Kapila.