Re: pg_upgrade and logical replication

On Tue, Nov 28, 2023 at 4:12 PM vignesh C <vignesh21@gmail.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?

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);
...
}

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.

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.

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.

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.

-- 
With Regards,
Amit Kapila.