Date: Sat, 7 Aug 2021 19:18:19 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Jeff Davis <pgsql@j-davis.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>, vignesh C <vignesh21@gmail.com>,
	pgsql-hackers@postgresql.org, Jacob Champion <pchampion@vmware.com>,
	Heikki Linnakangas <hlinnaka@iki.fi>,
	Ashwin Agrawal <aagrawal@pivotal.io>,
	Ashutosh Sharma <ashu.coek88@gmail.com>,
	Andres Freund <andres@anarazel.de>,
	Robert Haas <robertmhaas@gmail.com>,
	Amit Kapila <amit.kapila16@gmail.com>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Dilip Kumar <dilipbalaut@gmail.com>
Subject: Re: alter table set TABLE ACCESS METHOD
Message-ID: <YQ5d60+vyW2Eff6D@paquier.xyz>
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>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="Zn31ZabWyIZd1EDd"
Content-Disposition: inline
In-Reply-To: <87ca2262da500df70f12bb33e992197a0c903094.camel@j-davis.com>
Archived-At: 
 <https://www.postgresql.org/message-id/YQ5d60%2BvyW2Eff6D%40paquier.xyz>
Precedence: bulk


--Zn31ZabWyIZd1EDd
Content-Type: multipart/mixed; boundary="doAr64aZwRUGL4YW"
Content-Disposition: inline


--doAr64aZwRUGL4YW
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--doAr64aZwRUGL4YW
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="alter-table-hook-wip.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablec=
mds.c
index 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 =3D 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 =3D true;
 			if (tab->chgPersistence)
 				ereport(ERROR,
 						(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
@@ -5475,6 +5479,8 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqu=
eue, LOCKMODE lockmode,
 							 RecentXmin,
 							 ReadNextMultiXactId(),
 							 persistence);
+
+			InvokeObjectPostAlterHook(RelationRelationId, tab->relid, 0);
 		}
 		else
 		{
@@ -5493,6 +5499,16 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wq=
ueue, 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);
+			}
 		}
 	}
=20
@@ -5971,6 +5987,7 @@ ATGetQueueEntry(List **wqueue, Relation rel)
 	tab->newTableSpace =3D InvalidOid;
 	tab->newrelpersistence =3D RELPERSISTENCE_PERMANENT;
 	tab->chgPersistence =3D false;
+	tab->rewriteAttempted =3D false;
=20
 	*wqueue =3D lappend(*wqueue, tab);
=20
@@ -13660,6 +13677,7 @@ ATPrepSetAccessMethod(AlteredTableInfo *tab, Relati=
on rel, const char *amname)
=20
 	/* Check that the table access method exists */
 	amoid =3D get_table_am_oid(amname, false);
+	tab->rewriteAttempted =3D true;
=20
 	if (rel->rd_rel->relam =3D=3D amoid)
 		return;

--doAr64aZwRUGL4YW--

--Zn31ZabWyIZd1EDd
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmEOXesACgkQnvQgOdby
QH1S/hAApQvKi6Hb2C1WFeJjbpmp74G6BntzMuBSxcCT72tnlYlKJPKJtjVlhG+0
iEgO8uRNspfE5Vfx+SP00plfeix/HSaFWQX3CdDHtTcilkdE5HS98VuBvawscp0E
SvTuuTAEnvMO/V6R/yNa+DiE0PZyIRnNCnmPbgdsdFHroOhIwtSMt89FtFE3Sw8l
Siqw24SdPMytOjf6zl/6MfbAf98G65YQI68jCiXjRb1X6xdE1Edx7zNdUVoAZmn8
2nxHXd63GosAqr4WFExuxWVqRBZ8fC8DLokl4JfRHD65D43jq7AThQUqFuxTndIu
I9kRqSItE3VADGbsMpe15DAsAsj7V/G0xnGd2+GKi9h+gGrEGae4ct+Vx/CzffEb
A+tHXJKEFhaFeIdYmb/LWhyQ6sZRxJj9XHkSDWermSnzfLTXnUF5ctQWtRmF0k/G
/gKI7nHbKv1DJ1jYgyxmLQM56y9cavht3eq6EvoAEctZQBmdWxHBR8H0GcAytD6y
xrSJJvF7D5BzEK0ofpaQkoemGpxmn6xk0fmdL5YZrdhWdfSsjnBYDTwwIWDrjfI1
1kLT6f3KSfRcgU+pk5T7Bpt9ep4nDMViQbjzAa7uWfAfw9UULRjH4ADDy8hPVxiy
7Mk0GMthPmGBjiJb8rpsVtOrvNIAS+2VbPH6zcCgEd4ATWkpuyY=
=Y2dR
-----END PGP SIGNATURE-----

--Zn31ZabWyIZd1EDd--