. On Thu, May 14, 2026 at 2:23 PM vignesh C <[email protected]> wrote: > > On Mon, 11 May 2026 at 14:59, Shlok Kyal <[email protected]> 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. >
+1. We can keep the current behavior as-is since it only applies when allow_system_table_mods is enabled. The documentation already clearly warns about the associated risks, so this should be fine. thanks Shveta
