| From: | vignesh C <vignesh21(at)gmail(dot)com> |
|---|---|
| To: | Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> |
| Cc: | Dilip Kumar <dilipbalaut(at)gmail(dot)com>, Nisha Moond <nisha(dot)moond412(at)gmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, shveta malik <shveta(dot)malik(at)gmail(dot)com>, Peter Smith <smithpb2250(at)gmail(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, shveta malik <shvetamalik(at)gmail(dot)com> |
| Subject: | Re: Proposal: Conflict log history table for Logical Replication |
| Date: | 2026-05-14 08:53:14 |
| Message-ID: | CALDaNm2oYGeXq5mrS50b_SX4990pLx3N2Uts8KCBHf4iN2BA8A@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Mon, 11 May 2026 at 14:59, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
> I started reviewing the patches.
> Here are minor comments for 0001 patch:
>
> 1. If allow_system_table_mods=on we can add/drop columns of conflict log tables
> But the same for pg_toast or other catalog tables are prohibited. Also
> for other system tables we are getting following error.
>
> postgres=# ALTER TABLE pg_toast.pg_toast_16413 DROP COLUMN chunk_seq;
> ERROR: ALTER action DROP COLUMN cannot be performed on relation
> "pg_toast_16413"
>
> DETAIL: This operation is not supported for TOAST tables.
> postgres=# ALTER TABLE pg_publication DROP COLUMN pubname;
> ERROR: cannot drop column pubname of table pg_publication because it
> is required by the database system
> postgres=# ALTER TABLE pg_description DROP COLUMN description;
> ERROR: cannot drop column description of table pg_description because
> it is required by the database system
>
> postgres=# ALTER TABLE pg_conflict.pg_conflict_log_16408 DROP COLUMN relname;
> ALTER TABLE
>
> Should we prohibit it for conflict log tables as well?
The reason it fails for regular system catalogs is that
IsPinnedObject() returns true for them. Objects with OIDs less than
FirstUnpinnedObjectId(12000) are considered pinned, which includes the
core catalogs created during initdb. In such cases, PostgreSQL
immediately throws the following error:
/*
* If the target object is pinned, we can just error out immediately; it
* won't have any objects recorded as depending on it.
*/
if (IsPinnedObject(object->classId, object->objectId))
ereport(ERROR,
(errcode(ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST),
errmsg("cannot drop %s because it is required by the
database system",
getObjectDescription(object, false))));
The call chain is:
ATExecDropColumn -> performMultipleDeletions -> findDependentObjects
-> IsPinnedObject
However, the conflict log tables are not created during initdb; they
are created later during subscription creation. Therefore, they are
not considered pinned objects, IsPinnedObject() returns false, and the
DROP COLUMN operation is allowed.
I also noticed that ADD COLUMN is currently allowed on system tables
when allow_system_table_mods is enabled:
postgres=# SET allow_system_table_mods = on;
SET
postgres=# ALTER TABLE pg_description ADD COLUMN fake text;
ALTER TABLE
There are also cases where such operations lead to assertion failures.
For example:
postgres=# SET allow_system_table_mods = on;
SET
postgres=# alter table pg_type add column fake int;
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
The connection to the server was lost. Attempting reset: Failed.
TRAP: failed Assert("i >= 0 && i < tupdesc->natts"), File:
"../../../src/include/access/tupdesc.h", Line: 182, PID: 11443
postgres: vignesh postgres [local] ALTER
TABLE(ExceptionalCondition+0xba) [0x616a67fc753c]
postgres: vignesh postgres [local] ALTER TABLE(+0x7057fa) [0x616a67d067fa]
postgres: vignesh postgres [local] ALTER
TABLE(build_column_default+0x34) [0x616a67d08961]
postgres: vignesh postgres [local] ALTER TABLE(+0x3e8875) [0x616a679e9875]
postgres: vignesh postgres [local] ALTER TABLE(+0x3e34e8) [0x616a679e44e8]
postgres: vignesh postgres [local] ALTER TABLE(+0x3e2e24) [0x616a679e3e24]
The documentation also explicitly warns about this behavior at [1]:
Allows modification of the structure of system tables as well as
certain other risky actions on system tables. This is otherwise not
allowed even for superusers. Ill-advised use of this setting can cause
irretrievable data loss or seriously corrupt the database system.
Given this, I am not sure whether we should specifically prevent
dropping columns from conflict log tables when allow_system_table_mods
is enabled.
Rest of the comments are addressed in the v34 version patch posted at [2].
[1] - https://www.postgresql.org/docs/current/runtime-config-developer.html
[2] - https://www.postgresql.org/message-id/CALDaNm1ZOWAbv5WCsORPBqo7tjHn4f7E%2BB5LZhExfnPMs-zo9A%40mail.gmail.com
Regards,
Vignesh
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Etsuro Fujita | 2026-05-14 08:57:41 | Re: Fix array-element quoting in postgres_fdw import statistics |
| Previous Message | shveta malik | 2026-05-14 08:52:11 | Re: Proposal: Conflict log history table for Logical Replication |