On Fri, Jan 21, 2022 at 8:55 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
>
> On Fri, Jan 21, 2022 at 10:10 AM Masahiko Sawada <sawada.m...@gmail.com>
> wrote:
> >
> > On Fri, Jan 21, 2022 at 1:20 PM Amit Kapila <amit.kapil...@gmail.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.
Thank you for updating the 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?
Agree with these 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.
Agreed.
> For the
> streaming case, we might be able to ensure streaming even with lesser
> data. Can you please try that?
Yeah, after some tests, it's enough to insert 500 rows as follows:
INSERT INTO test_tab_streaming SELECT i, md5(i::text) FROM
generate_series(1, 500) s(i);
I've just sent another email about that probably we can remove two
tests for ROLLBACK PREPARED, so I’ll update the patch while including
this point.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
