From: Michael Paquier <michael@paquier.xyz>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Inconsistent error message wording for REINDEX CONCURRENTLY
Date: Wed, 8 May 2019 16:58:53 +0900
Message-ID: <20190508075853.GC1602@paquier.xyz> (raw)
In-Reply-To: <2321.1557263978@sss.pgh.pa.us>
References: <23694.1556806002@sss.pgh.pa.us>
<20190504085543.GE2174@paquier.xyz>
<4469.1556982011@sss.pgh.pa.us>
<20190505141645.GA7392@paquier.xyz>
<16468.1557077559@sss.pgh.pa.us>
<21697.1557092753@sss.pgh.pa.us>
<2321.1557263978@sss.pgh.pa.us>
On Tue, May 07, 2019 at 05:19:38PM -0400, Tom Lane wrote:
> That's nothing but a hack, and the reason it's necessary is that
> index_create will throw error if IsCatalogRelation is true, which
> it will be for information_schema TOAST tables --- but not for their
> parent tables that are being examined here.
Oh. Good catch. That's indeed crazy now that I look closer at that.
> After looking around, it seems to me that the correct definition for
> IsCatalogRelation is just "is the OID less than FirstBootstrapObjectId?".
> Currently we could actually restrict it to "less than
> FirstGenbkiObjectId", because all the catalogs, indexes, and TOAST tables
> have hand-assigned OIDs --- but perhaps someday we'll let genbki.pl
> assign some of those OIDs, so I prefer the weaker constraint. In any
> case, this gives us a correct separation between objects that are
> traceable to the bootstrap data and those that are created by plain SQL
> later in initdb.
>
> With this, the Form_pg_class argument to IsCatalogClass becomes
> vestigial. I'm tempted to get rid of that function altogether in
> favor of direct calls to IsCatalogRelationOid, but haven't done so
> in the attached.
I think that removing entirely IsCatalogClass() is just better as if
any extension uses this routine, then it could potentially simplify
its code because needing Form_pg_class means usually opening a
Relation, and this can be removed.
With IsCatalogClass() removed, the only dependency with Form_pg_class
comes from IsToastClass() which is not used at all except in
IsSystemClass(). Wouldn't it be better to remove entirely
IsToastClass() and switch IsSystemClass() to use a namespace OID
instead of Form_pg_class?
With your patch, ReindexRelationConcurrently() does not complain for
REINDEX TABLE CONCURRENTLY for a catalog table and would trigger the
error from index_create(), which is at the origin of this thread. The
check with IsSharedRelation() for REINDEX INDEX CONCURRENTLY is
useless and the error message generated for IsCatalogRelationOid()
still needs to be improved. Would you prefer to include those changes
in your patch? Or should I work on top of what you are proposing
(your patch does not include negative tests for toast index and
tables on catalogs either).
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: michael@paquier.xyz, tgl@sss.pgh.pa.us, pgsql-hackers@lists.postgresql.org
Subject: Re: Inconsistent error message wording for REINDEX CONCURRENTLY
In-Reply-To: <20190508075853.GC1602@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox