Re: pg_upgrade and logical replication

On Wed, 29 Nov 2023 at 15:02, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Nov 28, 2023 at 4:12 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
>
> Few comments on the latest patch:
> ===========================
> 1.
> + if (fout->remoteVersion >= 170000)
> + appendPQExpBufferStr(query, " o.remote_lsn AS suboriginremotelsn,\n");
> + else
> + appendPQExpBufferStr(query, " NULL AS suboriginremotelsn,\n");
> +
> + if (dopt->binary_upgrade && fout->remoteVersion >= 170000)
> + appendPQExpBufferStr(query, " s.subenabled\n");
> + else
> + appendPQExpBufferStr(query, " false AS subenabled\n");
> +
> + appendPQExpBufferStr(query,
> + "FROM pg_subscription s\n");
> +
> + if (fout->remoteVersion >= 170000)
> + appendPQExpBufferStr(query,
> + "LEFT JOIN pg_catalog.pg_replication_origin_status o \n"
> + " ON o.external_id = 'pg_' || s.oid::text \n");
>
> Why 'subenabled' have a check for binary_upgrade but
> 'suboriginremotelsn' doesn't?

Combined these two now.

> 2.
> +Datum
> +binary_upgrade_add_sub_rel_state(PG_FUNCTION_ARGS)
> +{
> + Relation rel;
> + HeapTuple tup;
> + Oid subid;
> + Form_pg_subscription form;
> + char *subname;
> + Oid relid;
> + char relstate;
> + XLogRecPtr sublsn;
> +
> + CHECK_IS_BINARY_UPGRADE;
> +
> + /* We must check these things before dereferencing the arguments */
> + if (PG_ARGISNULL(0) || PG_ARGISNULL(1) || PG_ARGISNULL(2))
> + elog(ERROR, "null argument to binary_upgrade_add_sub_rel_state is
> not allowed");
> +
> + subname = text_to_cstring(PG_GETARG_TEXT_PP(0));
> + relid = PG_GETARG_OID(1);
> + relstate = PG_GETARG_CHAR(2);
> + sublsn = PG_ARGISNULL(3) ? InvalidXLogRecPtr : PG_GETARG_LSN(3);
> +
> + tup = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
> + if (!HeapTupleIsValid(tup))
> + ereport(ERROR,
> + errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("relation %u does not exist", relid));
> + ReleaseSysCache(tup);
> +
> + rel = table_open(SubscriptionRelationId, RowExclusiveLock);
>
> Why there is no locking for relation? I see that during subscription
> operation, we do acquire AccessShareLock on the relation before adding
> a corresponding entry in pg_subscription_rel. See the following code:
>
> CreateSubscription()
> {
> ...
> foreach(lc, tables)
> {
> RangeVar *rv = (RangeVar *) lfirst(lc);
> Oid relid;
>
> relid = RangeVarGetRelid(rv, AccessShareLock, false);
>
> /* Check for supported relkind. */
> CheckSubscriptionRelkind(get_rel_relkind(relid),
> rv->schemaname, rv->relname);
>
> AddSubscriptionRelState(subid, relid, table_state,
> InvalidXLogRecPtr);
> ...
> }

Modified

> 3.
> +Datum
> +binary_upgrade_add_sub_rel_state(PG_FUNCTION_ARGS)
> {
> ...
> ...
> + AddSubscriptionRelState(subid, relid, relstate, sublsn);
> ...
> }
>
> I see a problem with directly using this function which is that it
> doesn't release locks which means it expects either the caller to
> release those locks or postpone to release them at the transaction
> end. However, all the other binary_upgrade support functions don't
> postpone releasing locks till the transaction ends. I think we should
> add an additional parameter to indicate whether we want to release
> locks and then pass it true from the binary upgrade support function.

Modified

> 4.
> extern void getPublicationTables(Archive *fout, TableInfo tblinfo[],
> int numTables);
> extern void getSubscriptions(Archive *fout);
> +extern void getSubscriptionTables(Archive *fout);
>
> getSubscriptions() and getSubscriptionTables() are defined in the
> opposite order in .c file. I think it is better to change the order in
> .c file unless there is a reason for not doing so.

Modified

> 5. At this stage, no need to update/send the 0002 patch, we can look
> at it after the main patch is committed. That is anyway not directly
> related to the main patch.

Removed it from this version.

> Apart from the above, I have modified a few comments and messages in
> the attached. Kindly review and include the changes if you are fine
> with those.

Merged them.

The attached v21 version patch has the change for the same.

Regards,
Vignesh