Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 20 Mar 2023 09:30:50 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Soumyadeep Chakraborty <soumyadeep2007@gmail.com>
Cc: Zhihong Yu <zyu@yugabyte.com>, Justin Pryzby <pryzby@telsasoft.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>,
	Ashwin Agrawal <ashwinstar@gmail.com>, vanjared@vmware.com
Subject: Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
Message-ID: <ZBepKJRdD2OUTz+O@paquier.xyz>
References: <CAE-ML+9zM4wJCGCBGv01k96qQ3gFv4WFcFy=zqPHKeaEFwwv6A@mail.gmail.com>
 <20220518231414.GN19626@telsasoft.com>
 <CAE-ML+_2dfVcxqxNOWooCLLLFRwaSda3_7uXXwsiXZNsh5TZyA@mail.gmail.com>
 <CALNJ-vRWNh87w0jHEep6zzB3rxJWe_TEm-YrJsixTOZ0PyTOdQ@mail.gmail.com>
 <CAE-ML+9NycfS3Q9PhMVpgB2NN62HMa9OBCwNW60AiO8NgCCNXw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="2ykndK8jgER2NvFn"
Content-Disposition: inline
In-Reply-To: <CAE-ML+9NycfS3Q9PhMVpgB2NN62HMa9OBCwNW60AiO8NgCCNXw@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/ZBepKJRdD2OUTz%2BO%40paquier.xyz>
Precedence: bulk


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

On Thu, Jun 09, 2022 at 11:21:58AM -0700, Soumyadeep Chakraborty wrote:
> Thanks. Fixed and rebased.

I think that I am OK with the concept of this patch to use a
partitioned table's relam as a reference when creating a partition
rather than relying on the default GUC, in a way similar to
tablespaces.

One worry I see is that forcing a recursion on the leaves on ALTER
TABLE could silently break partitions where multiple table AMs are
used across separate partitions if ALTER TABLE SET ACCESS METHOD is
used on one of the parents, though it seems like this is not something
I would much worry about as now the command is an error.

A second worry is that we would just break existing creation flows
that rely on the GUC defining the default AM.  This is worth a close
lookup at pg_dump to make sure that we do things correctly with this
patch in place..  Did you check dump and restore flows with partition
trees and --no-table-access-method?  Perhaps there should be
some regression tests with partitioned tables?

+    /*
+     * For partitioned tables, when no access method is specified, we
+     * default to the parent table's AM.
+     */
+    Assert(list_length(inheritOids) == 1);
+    /* XXX: should implement get_rel_relam? */
+    relid = linitial_oid(inheritOids);
+    tup = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
+    if (!HeapTupleIsValid(tup))
+            elog(ERROR, "cache lookup failed for relation %u", relid);

Having a wrapper for that could be useful, yes.  We don't have any
code paths that would directly need that now, from what I can see,
though.  This patch gives one reason to have one.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmQXqToACgkQnvQgOdby
QH2HPxAAi+Q5NA4fw9Rchiyk9DzF+hhdFnDzyHvheiAxAatvNTjVSPwz5jTqdoKm
YRHQ/DTPiCHoy8Ac9Ar8XkOY6EJLa1ba9f4PFieggoNl2icHH3tm4eQdTtbZ4dG1
ELy1nGNK0aLB3dYeV0DPZW1iLrWiz1j3aM2utAibZq32ARrO+uZaIuU/csUeDzNN
VCXnoncefbQ+718DEWVMUydvD+OzK8+ihjKLgA/PlcvkRB+DNFQy6tBAdleIvApA
r19wCC7fbw+Em3pbOYc4Qby3T8p4KVx+kv/BeC4JtGM2u6zptLyhYD4Aj6ATV6R1
Un2x/um7Y6KnchZs/7WGSxjI2kUSLirsbhoZSFvulb2Tj9JuLa0DOk6fcsz5Xl+4
Gsdv8iVRxz4l5/gaY+8UQX28toHm1BypfqfwiTetCa4JrsuI+9ydI5g/UN/L+6xl
CoA1TW//5brRihHGJzV2wbwAZl8p74daATWxpZjfN7YHb/fZmlFEtofQMtmsR59m
yP7M7tkoteR+hiEUqdDQm85uee4tBT4mnED+Z0cCKYuWgoDzgWqG/uO79/Bm+Vdj
TZc2Vkki5qkbz/htzEdocxsCXsSjVp0e0UoRdr/uAUP2hl3hbKLD1zhPdK5FBk9c
lZxs5Kfq8A5jBFjNFtdyARcVli5HWWbjaZm7PnZWNLz74O+3uGw=
=+bfY
-----END PGP SIGNATURE-----

--2ykndK8jgER2NvFn--