Date: Thu, 21 Mar 2019 17:17:04 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Jesper Pedersen <jesper.pedersen@redhat.com>
Cc: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>,
	Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: partitioned tables referenced by FKs
Message-ID: <20190321201704.GA8280@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20190318204235.GA30360@alvherre.pgsql>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On 2019-Mar-18, Alvaro Herrera wrote:

> > I'm getting a failure in the pg_upgrade test:
> > 
> >  --
> > +-- Name: pk5 pk5_pkey; Type: CONSTRAINT; Schema: regress_fk; Owner:
> > jpedersen
> > +--
> > +
> > +ALTER TABLE ONLY regress_fk.pk5
> > +    ADD CONSTRAINT pk5_pkey PRIMARY KEY (a);
> > +
> > +
> > +--
> > 
> > when running check-world.
> 
> So I tested this case just now (after fixing a couple of bugs) and see a
> slightly different problem now: those excess lines are actually just
> that pg_dump chose to print the constraints in different order, as there
> are identical lines with "-" in the diff I get.  The databases actually
> *are* identical as far as I can tell.  I haven't yet figured out how to
> fix this; of course, the easiest solution is to just drop the regress_fk
> schema at the end of the foreign_key.sql regression test, but I would
> prefer a different solution.

I figured this out.  It's actually a bug in pg11, whereby we're setting
a dependency wrongly.  If you try to do pg_describe_object() the
pg_depend entries for tables set up the way the regression test does it,
it'll fail with a "cache lookup failed for constraint XYZ".  In other
words, pg_depend contains bogus data :-(

Here's one possible fix:

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index c339a2bb779..8f62b454f75 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1357,6 +1357,8 @@ index_constraint_create(Relation heapRelation,
 	 */
 	if (OidIsValid(parentConstraintId))
 	{
+		ObjectAddress	referenced;	/* XXX shadow outer variable */
+
 		ObjectAddressSet(myself, ConstraintRelationId, conOid);
 		ObjectAddressSet(referenced, ConstraintRelationId, parentConstraintId);
 		recordDependencyOn(&myself, &referenced, DEPENDENCY_PARTITION_PRI);

I can tell the raised eyebrows from a mile away :-(

The problem (and the reason shadowing the variable solves it) is that
the outer 'referenced' is the function's return value.  This block was
clobbering the value previously set, which is what we really wanted to
return.

/me dons paper bag

I'll go figure out what a better solution is.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services