On Tue, 20 Feb 2024 at 15:47, Hayato Kuroda (Fujitsu)
<kuroda.hay...@fujitsu.com> wrote:
>
> Dear Vignesh,
>
> Thanks for giving comments!
>
> > Few comments for v22-0001 patch:
> > 1) The second "if (strcmp(PQgetvalue(res, 0, 1), "t") != 0)"" should
> > be if (strcmp(PQgetvalue(res, 0, 2), "t") != 0):
> > + if (strcmp(PQgetvalue(res, 0, 1), "t") != 0)
> > + {
> > + pg_log_error("permission denied for database %s",
> > dbinfo[0].dbname);
> > + return false;
> > + }
> > + if (strcmp(PQgetvalue(res, 0, 1), "t") != 0)
> > + {
> > + pg_log_error("permission denied for function \"%s\"",
> > +
> > "pg_catalog.pg_replication_origin_advance(text, pg_lsn)");
> > + return false;
> > + }
>
> I have already pointed out as comment #8 [1] and fixed in v22-0005.
>
> > 2) pg_createsubscriber fails if a table is parallely created in the
> > primary node:
> > 2024-02-20 14:38:49.005 IST [277261] LOG: database system is ready to
> > accept connections
> > 2024-02-20 14:38:54.346 IST [277270] ERROR: relation "public.tbl5"
> > does not exist
> > 2024-02-20 14:38:54.346 IST [277270] STATEMENT: CREATE SUBSCRIPTION
> > pg_createsubscriber_5_277236 CONNECTION ' dbname=postgres' PUBLICATION
> > pg_createsubscriber_5 WITH (create_slot = false, copy_data = false,
> > enabled = false)
> >
> > If we are not planning to fix this, at least it should be documented
>
> The error will be occurred when tables are created after the promotion, right?
> I think it cannot be fixed until DDL logical replication would be implemented.
> So, +1 to add descriptions.
>
> > 3) Error conditions is verbose mode has invalid error message like
> > "out of memory" messages like in below:
> > pg_createsubscriber: waiting the postmaster to reach the consistent state
> > pg_createsubscriber: postmaster reached the consistent state
> > pg_createsubscriber: dropping publication "pg_createsubscriber_5" on
> > database "postgres"
> > pg_createsubscriber: creating subscription
> > "pg_createsubscriber_5_278343" on database "postgres"
> > pg_createsubscriber: error: could not create subscription
> > "pg_createsubscriber_5_278343" on database "postgres": out of memory
>
> Because some places use PQerrorMessage() wrongly. It should be
> PQresultErrorMessage(). Fixed in v22-0005.
>
> > 4) In error cases we try to drop this publication again resulting in error:
> > + /*
> > + * Since the publication was created before the
> > consistent LSN, it is
> > + * available on the subscriber when the physical
> > replica is promoted.
> > + * Remove publications from the subscriber because it
> > has no use.
> > + */
> > + drop_publication(conn, &dbinfo[i]);
> >
> > Which throws these errors(because of drop publication multiple times):
> > pg_createsubscriber: dropping publication "pg_createsubscriber_5" on
> > database "postgres"
> > pg_createsubscriber: error: could not drop publication
> > "pg_createsubscriber_5" on database "postgres": ERROR: publication
> > "pg_createsubscriber_5" does not exist
> > pg_createsubscriber: dropping publication "pg_createsubscriber_5" on
> > database "postgres"
> > pg_createsubscriber: dropping the replication slot
> > "pg_createsubscriber_5_278343" on database "postgres"
>
> Right. One approach is to use DROP PUBLICATION IF EXISTS statement.
> Thought?
Another way would be to set made_publication to false in
drop_publication once the publication is dropped. This way after the
publication is dropped it will not try to drop the publication again
in cleanup_objects_atexit as the made_publication will be false now.
Regards,
Vignesh
