From: Michael Paquier <michael@paquier.xyz>
To: Jeff Davis <pgsql@j-davis.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>
Cc: vignesh C <vignesh21@gmail.com>
Cc: pgsql-hackers@postgresql.org, Jacob Champion <pchampion@vmware.com>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Ashwin Agrawal <aagrawal@pivotal.io>
Cc: Ashutosh Sharma <ashu.coek88@gmail.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Dilip Kumar <dilipbalaut@gmail.com>
Subject: Re: alter table set TABLE ACCESS METHOD
Date: Sat, 7 Aug 2021 19:18:19 +0900
Message-ID: <YQ5d60+vyW2Eff6D@paquier.xyz> (raw)
In-Reply-To: <87ca2262da500df70f12bb33e992197a0c903094.camel@j-davis.com>
References: <20210722094154.GD19620@telsasoft.com>
<YP+4CEOMofngeDJi@paquier.xyz>
<YQDNwJp3amIJKizD@paquier.xyz>
<10785e3e9456a5d761164d3e60d9c4981b80e321.camel@j-davis.com>
<YQDk8UNge1b8dJXA@paquier.xyz>
<bfad9843733672c468783532fd440731aceb1f8f.camel@j-davis.com>
<YQJKV29/1a60uG68@paquier.xyz>
<d6a66a0ab44b43e416ee95aeab32d6b8afab8d0a.camel@j-davis.com>
<YQOoq17l7qvTBg1g@paquier.xyz>
<87ca2262da500df70f12bb33e992197a0c903094.camel@j-davis.com>
On Fri, Jul 30, 2021 at 02:18:02PM -0700, Jeff Davis wrote:
> It sounds like anything we do here should be part of a larger change to
> make it consistent. So I'm fine with the patch you posted.
As a matter of curiosity, I have checked how it would look to handle
the no-op case for the sub-commands other than SET TABLESPACE, and one
would need something like the attached, with a new flag for
AlteredTableInfo. That does not really look good, but it triggers
properly the object access hook when SET LOGGED/UNLOGGED/ACCESS METHOD
are no-ops, so let's just handle the case using the version from
upthread. I'll do that at the beginning of next week.
--
Michael
Attachments:alter-table-hook-wip.patch (text/x-diff, 2.7K) downloaddiff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.cindex fcd778c62a..eecbde8479 100644--- a/src/backend/commands/tablecmds.c+++ b/src/backend/commands/tablecmds.c@@ -179,6 +179,8 @@ typedef struct AlteredTableInfo
Oid newAccessMethod; /* new access method; 0 means no change */
Oid newTableSpace; /* new tablespace; 0 means no change */
bool chgPersistence; /* T if SET LOGGED/UNLOGGED is used */
+ bool rewriteAttempted; /* T if a rewrite operation was attempted,+ * may be a no-op */
char newrelpersistence; /* if above is true */
Expr *partition_constraint; /* for attach partition validation */
/* true, if validating default due to some other attach/detach */
@@ -4593,6 +4595,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
break;
case AT_SetLogged: /* SET LOGGED */
ATSimplePermissions(cmd->subtype, rel, ATT_TABLE);
+ tab->rewriteAttempted = true;
if (tab->chgPersistence)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
@@ -4608,6 +4611,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
break;
case AT_SetUnLogged: /* SET UNLOGGED */
ATSimplePermissions(cmd->subtype, rel, ATT_TABLE);
+ tab->rewriteAttempted = true;
if (tab->chgPersistence)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
@@ -5475,6 +5479,8 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode,
RecentXmin,
ReadNextMultiXactId(),
persistence);
++ InvokeObjectPostAlterHook(RelationRelationId, tab->relid, 0);
}
else
{
@@ -5493,6 +5499,16 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode,
*/
if (tab->newTableSpace)
ATExecSetTableSpace(tab->relid, tab->newTableSpace, lockmode);
+ else if (tab->rewriteAttempted)+ {+ /*+ * Even if no rewrite is going to happen, it may be possible+ * that one has gone through SET LOGGED/UNLOGGED or ACCESS+ * METHOD while being a no-op. If that's the case, invoke the+ * access hook.+ */+ InvokeObjectPostAlterHook(RelationRelationId, tab->relid, 0);+ }
}
}
@@ -5971,6 +5987,7 @@ ATGetQueueEntry(List **wqueue, Relation rel)
tab->newTableSpace = InvalidOid;
tab->newrelpersistence = RELPERSISTENCE_PERMANENT;
tab->chgPersistence = false;
+ tab->rewriteAttempted = false;
*wqueue = lappend(*wqueue, tab);
@@ -13660,6 +13677,7 @@ ATPrepSetAccessMethod(AlteredTableInfo *tab, Relation rel, const char *amname)
/* Check that the table access method exists */
amoid = get_table_am_oid(amname, false);
+ tab->rewriteAttempted = true;
if (rel->rd_rel->relam == amoid)
return;
signature.asc (application/pgp-signature, 832B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: michael@paquier.xyz, pgsql@j-davis.com, pryzby@telsasoft.com, vignesh21@gmail.com, pchampion@vmware.com, hlinnaka@iki.fi, aagrawal@pivotal.io, ashu.coek88@gmail.com, andres@anarazel.de, robertmhaas@gmail.com, amit.kapila16@gmail.com, alvherre@alvh.no-ip.org, dilipbalaut@gmail.com
Subject: Re: alter table set TABLE ACCESS METHOD
In-Reply-To: <YQ5d60+vyW2Eff6D@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox