From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@postgreSQL.org
Subject: FieldSelect/FieldStore dependencies, take 2
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <22474.1509063971.0@sss.pgh.pa.us>
Date: Thu, 26 Oct 2017 20:29:06 -0400
Message-ID: <22571.1509064146@sss.pgh.pa.us>
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <22474.1509063971.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable

I had a nagging feeling that commit f3ea3e3e8 was not quite covering
all the bases with respect to what dependencies to record for
FieldSelect/FieldStore nodes: it looked at the result type, but what
about the input type?  Just now, while fooling around with domains
over composite, I stumbled across a case that shows what's missing:

regression=3D# create type complex as (r float8, i float8);
CREATE TYPE
regression=3D# create domain dcomplex as complex check((value).r > (value).=
i);
CREATE DOMAIN
regression=3D# select pg_get_constraintdef((select max(oid) from pg_constra=
int));
      pg_get_constraintdef=20=20=20=20=20=20=20
---------------------------------
 CHECK (((VALUE).r > (VALUE).i))
(1 row)

regression=3D# alter type complex drop attribute r;
ALTER TYPE
regression=3D# select pg_get_constraintdef((select max(oid) from pg_constra=
int));
                     pg_get_constraintdef=20=20=20=20=20=20=20=20=20=20=20=
=20=20=20=20=20=20=20=20=20=20
--------------------------------------------------------------
 CHECK (((VALUE)."........pg.dropped.1........" > (VALUE).i))
(1 row)

Nothing seems to crash at this point, probably because we insert
nulls into dropped columns, so the CHECK just sees a NULL value
for "r" whenever it runs.  But obviously, the next dump/reload
or pg_upgrade is not going to end well.

So what this shows is that we need a dependency on the particular
column named by the FieldSelect or FieldStore.  Under normal
circumstances, that obviates the need for a dependency on the
FieldSelect's result type, which would match the column type.
I think concretely what we need is the attached.

(BTW, the getBaseType() is only necessary in HEAD, since before
domains-over-composites the argument of a FieldSelect couldn't
be a domain type.)

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="fieldselect-dependencies-take-two.patch";
	charset="us-ascii"
Content-ID: <22474.1509063971.2@sss.pgh.pa.us>
Content-Description: fieldselect-dependencies-take-two.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/depende=
ncy.c
index 3b214e5..033c435 100644
*** a/src/backend/catalog/dependency.c
--- b/src/backend/catalog/dependency.c
*************** find_expr_references_walker(Node *node,
*** 1716,1725 ****
  	else if (IsA(node, FieldSelect))
  	{
  		FieldSelect *fselect =3D (FieldSelect *) node;
=20=20
! 		/* result type might not appear anywhere else in expression */
! 		add_object_address(OCLASS_TYPE, fselect->resulttype, 0,
! 						   context->addrs);
  		/* the collation might not be referenced anywhere else, either */
  		if (OidIsValid(fselect->resultcollid) &&
  			fselect->resultcollid !=3D DEFAULT_COLLATION_OID)
--- 1716,1739 ----
  	else if (IsA(node, FieldSelect))
  	{
  		FieldSelect *fselect =3D (FieldSelect *) node;
+ 		Oid			argtype =3D getBaseType(exprType((Node *) fselect->arg));
+ 		Oid			reltype =3D get_typ_typrelid(argtype);
=20=20
! 		/*
! 		 * We need a dependency on the specific column named in FieldSelect,
! 		 * assuming we can identify the pg_class OID for it.  (Probably we
! 		 * always can at the moment, but in future it might be possible for
! 		 * argtype to be RECORDOID.)  If we can make a column dependency then
! 		 * we shouldn't need a dependency on the column's type; but if we
! 		 * can't, make a dependency on the type, as it might not appear
! 		 * anywhere else in the expression.
! 		 */
! 		if (OidIsValid(reltype))
! 			add_object_address(OCLASS_CLASS, reltype, fselect->fieldnum,
! 							   context->addrs);
! 		else
! 			add_object_address(OCLASS_TYPE, fselect->resulttype, 0,
! 							   context->addrs);
  		/* the collation might not be referenced anywhere else, either */
  		if (OidIsValid(fselect->resultcollid) &&
  			fselect->resultcollid !=3D DEFAULT_COLLATION_OID)
*************** find_expr_references_walker(Node *node,
*** 1729,1738 ****
  	else if (IsA(node, FieldStore))
  	{
  		FieldStore *fstore =3D (FieldStore *) node;
=20=20
! 		/* result type might not appear anywhere else in expression */
! 		add_object_address(OCLASS_TYPE, fstore->resulttype, 0,
! 						   context->addrs);
  	}
  	else if (IsA(node, RelabelType))
  	{
--- 1743,1762 ----
  	else if (IsA(node, FieldStore))
  	{
  		FieldStore *fstore =3D (FieldStore *) node;
+ 		Oid			reltype =3D get_typ_typrelid(fstore->resulttype);
=20=20
! 		/* similar considerations to FieldSelect, but multiple column(s) */
! 		if (OidIsValid(reltype))
! 		{
! 			ListCell   *l;
!=20
! 			foreach(l, fstore->fieldnums)
! 				add_object_address(OCLASS_CLASS, reltype, lfirst_int(l),
! 								   context->addrs);
! 		}
! 		else
! 			add_object_address(OCLASS_TYPE, fstore->resulttype, 0,
! 							   context->addrs);
  	}
  	else if (IsA(node, RelabelType))
  	{

------- =_aaaaaaaaaa0
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

------- =_aaaaaaaaaa0--