Date: Tue, 25 Apr 2017 11:42:47 -0400
From: Stephen Frost <sfrost@snowman.net>
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Cc: Robert Haas <robertmhaas@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>,
	Noah Misch <noah@leadboat.com>,
	Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: pg_dump emits ALTER TABLE ONLY partitioned_table
Message-ID: <20170425154247.GJ9812@tamriel.snowman.net>
References: <20170411131233.GQ9812@tamriel.snowman.net>
 <4c4c2400-430b-2e78-3ce2-e7fede4353b4@lab.ntt.co.jp>
 <CA+Tgmob=xywPTSnok_aObFW7OssZi0VV-ke-MSN4hfaz437_3w@mail.gmail.com>
 <20170412192930.GJ9812@tamriel.snowman.net>
 <CA+TgmoY7d=Tr4pYKnvqG4c174dXazwtQeNm6FJCj1y0oqvBofA@mail.gmail.com>
 <0fae1d55-337b-d7e1-d420-b3a617fc7383@lab.ntt.co.jp>
 <20170417164325.GY9812@tamriel.snowman.net>
 <399b8962-b761-528e-1008-a9643371c984@lab.ntt.co.jp>
 <20170420234334.GW9812@tamriel.snowman.net>
 <9b7baae3-21b5-a730-1ec4-aee290d54518@lab.ntt.co.jp>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="6mOd0PKKueeDsiu5"
Content-Disposition: inline
In-Reply-To: <9b7baae3-21b5-a730-1ec4-aee290d54518@lab.ntt.co.jp>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org


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

Amit,

* Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote:
> I think why getPartitions() is separate from getInherits() and then
> flagPartitions() separate from flagInhTables() is because I thought
> originally that mixing the two would be undesirable.  In the partitioning
> case, getPartitions() joins pg_inherits with pg_class so that it can also
> get hold of relpartbound, which I then thought would be a bad idea to do
> for all of the inheritance tables.  If we're OK with always doing the
> join, I don't see why we couldn't get rid of the separation.

I'm not sure what you mean here.  We're always going to call both
getInherits() and getPartitions() and run the queries in each, with the
way the code is written today.  In my experience working with pg_dump
and trying to not slow it down, the last thing we want to do is run two
independent queries when one will do.  Further, we aren't really
avoiding any work when we have to go look at pg_class to exclude the
partitioned tables in getInherits() anyway.  I'm not seeing why we
really need the join at all though, see below.

> flagInhAttrs(), OTOH, seems unaffected by that concern and I think using
> it for both inheritance and partitioning is fine.  It affects NOT NULL
> constraints and defaults, which as far as it goes, applies to both
> inheritance and partitioning the same.

I don't see why we need to have getPartitions(), flagPartitions(), or
findPartitonParentByOid().  They all seem to be largely copies of the
inheritance functions, but it doesn't seem like that's really necessary
or sensible.

I also don't follow why we're pulling the partbound definition in
getPartitions() instead of in getTables(), where we're already pulling
the rest of the data from pg_class?  Or why getTablePartitionKeyInfo()
exists instead of also doing that during getTables()?  I looked through
pg_get_partkeydef() and it didn't seem to be particularly expensive to
run, though evidently it doesn't handle being passed an OID that it
doesn't expect very cleanly:

=# select pg_get_partkeydef(oid) from pg_class;
ERROR:  cache lookup failed for partition key of 52333

I don't believe that's really very appropriate of it to do though and
instead it should work the same way pg_get_indexdef() and others do and
return NULL in such cases, so people can use it sanely.

I'm working through this but it's going to take a bit of time to clean
it up and it's not going to get finished today, but I do think it needs
to get done and so I'll work to make it happen this week.  I didn't
anticipate finding this, obviously and am a bit disappointed by it.

> So, the newly added tests seem enough for now?

Considering the pg_upgrade regression test didn't pass with the patch
as sent, I'd say that more tests are needed.  I will be working to add
those also.

Thanks!

Stephen

--6mOd0PKKueeDsiu5
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: Digital signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCgAGBQJY/253AAoJEO1sijiDR2RVF+oP/iz3xDHoFJtU3cID5vMegmn1
zx4j078FWj5rBFXrCkRwPgbd2Qmy1MORqxF57UmUlVleZcFbv2LJaR16NgqiML0c
92D21VhmvbSrRJmQ/nAdgWygAdcVzyeWLynFYEJoQiH0O1YjPDhqm8j+jSB4q0qq
AF7MbqeoXTUUhCBnYhk5nxMsDrYZYpcadTqbXVRKhyTNVsOXSHWkcd9Q3q0TLwyG
G0EQ62IEdwjGexisC1vGkKo+lFfNyyC9uVDx2K/gca3l+1cVMVnTpceX2VqNuCSm
kZy9+36+o64BSKpfyrtA0G9cZsve3f7FLlv5jEb3/z46QGQHD5pbaZIr7caXHCWD
OdqseW+BBXqgATTbnszRo26GTFAhE62Ugzxhyee6p72bX/XYR8saPEpOPPUIhb2z
zP2bNhKwHf3WuVYIwTsPRUeC+4Fst0DywOR3xT8H8bRglMqWj/flpYGZYbDdmSz7
U3/ore7v6tzplXMmj/1ht2Us6grUwA170gqaDv2ZULTIFCq4ikwxp/3RHj2tM+CE
ak08hFzL8Tr53Xp3bhpFzLTl4NcNXOUC2NDkCgHgLpd/KteJy/B9ocAMePDAPo5P
O2FHZHINzCqlkd97Zc7bDfwoPlMWeky3QH5bxz1L16FCV9fbwSgPdd+75cQLP0Lm
J5fE5L8BZyF2y34Q7OiN
=bOHk
-----END PGP SIGNATURE-----

--6mOd0PKKueeDsiu5--