Re: pg_upgrade and logical replication
От | Amit Kapila |
---|---|
Тема | Re: pg_upgrade and logical replication |
Дата | |
Msg-id | CAA4eK1KmvGfWgrM7dm3W8PF623KKhKG5siNDZ-dNs-3UkPAFag@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: pg_upgrade and logical replication (vignesh C <vignesh21@gmail.com>) |
Ответы |
Re: pg_upgrade and logical replication
(Amit Kapila <amit.kapila16@gmail.com>)
Re: pg_upgrade and logical replication (vignesh C <vignesh21@gmail.com>) |
Список | pgsql-hackers |
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.
Вложения
В списке pgsql-hackers по дате отправления:
Предыдущее
От: "Hayato Kuroda (Fujitsu)"Дата:
Сообщение: RE: [PoC] pg_upgrade: allow to upgrade publisher node
Следующее
От: tender wangДата:
Сообщение: Re: SLRU optimization - configurable buffer pool and partitioning the SLRU lock