Date: Thu, 8 Dec 2016 15:41:06 -0500
From: Stephen Frost <sfrost@snowman.net>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers@postgresql.org
Subject: Re: pg_dump vs. TRANSFORMs
Message-ID: <20161208204106.GI23417@tamriel.snowman.net>
References: <20160504183952.GE10850@tamriel.snowman.net>
 <20161207200915.GZ23417@tamriel.snowman.net>
 <729.1481151371@sss.pgh.pa.us>
 <20161208142116.GC23417@tamriel.snowman.net>
 <4955.1481209396@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="ukZ3tWq1IjZw6zEx"
Content-Disposition: inline
In-Reply-To: <4955.1481209396@sss.pgh.pa.us>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org


--ukZ3tWq1IjZw6zEx
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> I have a vague feeling that the code for dumping casts and/or transfor=
ms
> >> may have some assumptions that the underlying function is also being
> >> dumped.  Although maybe the assumption was really only what's fixed he=
re,
> >> ie that there be a DumpableObject for the function.  Anyway, take a cl=
ose
> >> look for that.
>=20
> > I'll look around and see, though my hunch is that, at some point, we
> > were just pulling all functions and then an optimization was added to
> > exclude pg_catalog and no one noticed that it broke casts using built-in
> > functions.
>=20
> Nah, that's historical revisionism --- the exclusion for system functions
> is very ancient.  It certainly predates transforms altogether, and
> probably predates the cast-dumping code in anything like its current form.

Apparently, that exclusion goes back to 7.3.

> I think the expectation was that casts using built-in functions were
> also built-in and so needn't be dumped.  There may be a comment about it
> somewhere, which would need to be revised now.

I don't see anything in current code or back to 9.2.  Going back
farther, I do find comments about skipping casts which only refer to
things in pg_* namespaces, which, of course, isn't correct either.

As such, creating a cast like so:

create cast (timestamptz as interval) with function age(timestamptz) as
assignment;

results in a cast which no version of pg_dump will actually dump out of
any PG server version since CREATE CAST was added.  I spent the effort
to get all the way back to 7.1 up and running, tho CREATE CAST wasn't
actually added until 7.3.

> (Actually, the most likely way in which this would break things is if
> it started causing built-in casts to get dumped ... have you checked?)

So, this is fun.  Apparently casts had OIDs > FirstNormalObjectId back
in 8.0 and earlier, so pg_dump >=3D 9.0 dumps out all casts which don't
have functions for server versions 7.3-8.0.  Casts which do have a
function aren't included though, be they user-defined or not, because
they're excluded by getFuncs() and dumpCast() just punts.

With my change, pg_dump'ing against 8.0 and earlier will dump out all
casts, including those with functions, since the function definitions
will now be pulled in for them by getFuncs().

What isn't clear to me is what to do about this.  Given the lack of
anyone complaining, and that this would at least ensure that the
user-defined casts are dumped, we could just go with this change and
tell people who are dumping against 8.0 and earlier databases to ignore
the errors from the extra CREATE CAST commands (they shouldn't hurt
anything, after all) during the restore.

Older versions of pg_dump don't have much to offer us regarding this
case- they didn't dump out user-defined casts which only used components
=66rom pg_catalog either.

Ok, thinking a bit more on this and testing, it looks like we record the
initdb-defined casts as 'pinned' in pg_depend all the way back to 7.3.
Therefore, we could use that as the gating factor for getFuncs() instead
of FirstNormalObjectId and then I can also modify getCasts() to exclude
pinned casts, which should fix pg_dump against 8.0 and earlier.

I'll start working on a patch for that, please let me know if you see
any huge glaring holes in my thinking.

Thanks!

Stephen

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

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

iQIcBAEBCgAGBQJYScViAAoJEO1sijiDR2RV8sQP/2e2TCX7sams56oxEwzl9Dur
vZjvMnItuMIiljEiDkyHkvgwhLQynOonV2r4CiQ24esQ/h/0wDcIFv/Rqsp249AF
BnHjN3IS5vZ7YmZ4vSvIMKzR38h9be1JxlfFQs349JappHHpOSDponWyvvw8S8oG
i3oYkJp1SPyIAGrJ/U5CSApzIU20UgAhdp6WGO5ZskxcTXdFRRZ2zxzjy13pud93
FEutvCfrhhQVPtxIDk2Mm63i2AB7Zfb/ck7/AxmdwN4KHwglXcFieDwV/deJUwSt
/fMPRp1iLiYauRbEgXIMuR94up1/e7EKeIi1XMe+XIep9kh97R89KWWPRkgpwS+L
fYTHqX5oNKmIyH/ppHOC+e+QaqET0g57ADMQ5mleDogwVLiRaKDe/Knqq35sHRJZ
zFoYdIUcoIC4iXtBQKBff4TrQH6U0g+NzefCJ1bRhKLRlN9We8ll766guHeCn/P+
mZtm0YIu3pRmY2fTY82i9xXMfxYupNG9XXV1wNwF9nk7xo9chRfa867aJdaJTUW8
RLOuT/28mKFU5zofRJ+JCk70okbhdoS32iYFySh/UWo7CUVbU+HLGVqCrWuiHSqa
mI+2VOdwj68mhcI6f4iMRXLEyRtit7YdwtgS5kxZ4EJU73gqcFxy3oP22pesarx5
L4StUe5kZAUhw+jo8+RR
=vMzr
-----END PGP SIGNATURE-----

--ukZ3tWq1IjZw6zEx--