From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
cc: Wells Oliver <wells.oliver@gmail.com>, Stephen Frost <sfrost@snowman.net>,
        pgsql-admin <pgsql-admin@postgresql.org>
Subject: Re: Odd pg dump error: cache lookup failure
In-reply-to: <20201022190155.GA29948@alvherre.pgsql>
References: <20201022190155.GA29948@alvherre.pgsql>
Comments: In-reply-to Alvaro Herrera <alvherre@alvh.no-ip.org>
	message dated "Thu, 22 Oct 2020 16:01:55 -0300"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <40484.1603397064.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Thu, 22 Oct 2020 16:04:24 -0400
Message-ID: <40485.1603397064@sss.pgh.pa.us>
Precedence: bulk

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2020-Oct-21, Tom Lane wrote:
>> I think the "test for that capability" bit needs more subtlety.

> Great ideas, thanks -- all adopted in the attached version.  I didn't
> test this with 9.5 but as you say NOWAIT is already supported there and
> the command itself does work.

OK, looking a bit harder this time:

1. I think this bit in LockViewRecurse_walker must be removed as well:

            /* Currently, we only allow plain tables or views to be locked=
. */
            if (relkind !=3D RELKIND_RELATION && relkind !=3D RELKIND_PART=
ITIONED_TABLE &&
                relkind !=3D RELKIND_VIEW)
                continue;

(Mostly, stuff in views would be ordinary tables anyway ... but
foreign tables are a possible exception.)

2. Should we add anything to the LOCK TABLE reference page?  I see nothing
there now that bears on this change, but ...

3. Is hard-coding #define's for ERRCODEs actually the best we can do here?
I guess it is right now, but ugh.  Seems like we ought to find a way for
frontend code to make use of errcodes.h.  Not a matter for this patch
though.

4. You neglected to change the table name in the warn_or_exit_horribly
error message.

> Testing for a view as I was doing was not good, because PG12 does allow
> locks on views.  I decided to use pg_aggregate_fnoid_index because it's
> easier for manual tests: it doesn't prevent connecting to the database
> when a strong lock is held.  This in contrast with other more obvious
> candidates.  It's a pretty arbitrary choice but it shouldn't harm
> anything since we don't actually hold the lock for more than an instant,
> and that only if it's not contended.

There might be some value in using one of pg_class's indexes instead,
mainly because the parser will surely need to touch that on the way
to performing the LOCK, while pg_aggregate is far afield.  Given the
limited range of PG versions that we'll ever need to perform the check
for, I don't think there's much risk in using any well-known index
from a version-compatibility standpoint; but acquiring locks across
different catalogs could conceivably risk some issues vs a VACUUM
FULL or the like.

Other than these points, it seems committable.

			regards, tom lane