pgsql: Remove objname/objargs split for referring to objects
Remove objname/objargs split for referring to objects
In simpler times, it might have worked to refer to all kinds of objects
by a list of name components and an optional argument list. But this
doesn't work for all objects, which has resulted in a collection of
hacks to place various other nodes types into these fields, which have
to be unpacked at the other end. This makes it also weird to represent
lists of such things in the grammar, because they would have to be lists
of singleton lists, to make the unpacking work consistently. The other
problem is that keeping separate name and args fields makes it awkward
to deal with lists of functions.
Change that by dropping the objargs field and have objname, renamed to
object, be a generic Node, which can then be flexibly assigned and
managed using the normal Node mechanisms. In many cases it will still
be a List of names, in some cases it will be a string Value, for types
it will be the existing Typename, for functions it will now use the
existing ObjectWithArgs node type. Some of the more obscure object
types still use somewhat arbitrary nested lists.
Reviewed-by: Jim Nasby <Jim.Nasby@BlueTreble.com>
Reviewed-by: Michael Paquier <michael.paquier@gmail.com>
Branch
------
master
Details
-------
http://git.postgresql.org/pg/commitdiff/8b6d6cf853aab12f0dc2adba7c99c3e458662734
Modified Files
--------------
src/backend/catalog/objectaddress.c | 447 +++++++++++++++------------
src/backend/commands/alter.c | 33 +-
src/backend/commands/comment.c | 11 +-
src/backend/commands/dropcmds.c | 175 +++++------
src/backend/commands/extension.c | 13 +-
src/backend/commands/seclabel.c | 4 +-
src/backend/commands/tablecmds.c | 10 +-
src/backend/commands/typecmds.c | 2 +-
src/backend/nodes/copyfuncs.c | 16 +-
src/backend/nodes/equalfuncs.c | 16 +-
src/backend/parser/gram.y | 440 ++++++++++++--------------
src/backend/parser/parse_utilcmd.c | 14 +-
src/include/catalog/objectaddress.h | 8 +-
src/include/commands/extension.h | 2 +-
src/include/nodes/parsenodes.h | 24 +-
src/test/regress/expected/event_trigger.out | 3 -
src/test/regress/expected/object_address.out | 24 +-
src/test/regress/sql/event_trigger.sql | 3 -
18 files changed, 610 insertions(+), 635 deletions(-)
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
Peter Eisentraut wrote:
Remove objname/objargs split for referring to objects
I don't know if this is the guilty commit, but I'm now getting these
compiler warnings:
/pgsql/source/master/src/backend/catalog/objectaddress.c: In function 'get_object_address':
/pgsql/source/master/src/backend/catalog/objectaddress.c:1624:33: warning: 'typenames[1]' may be used uninitialized in this function [-Wmaybe-uninitialized]
ereport(ERROR,
^
/pgsql/source/master/src/backend/catalog/objectaddress.c:1578:8: note: 'typenames[1]' was declared here
char *typenames[2];
^
/pgsql/source/master/src/backend/catalog/objectaddress.c:1624:33: warning: 'typenames[0]' may be used uninitialized in this function [-Wmaybe-uninitialized]
ereport(ERROR,
^
/pgsql/source/master/src/backend/catalog/objectaddress.c:1578:8: note: 'typenames[0]' was declared here
char *typenames[2];
^
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
On Thu, Mar 16, 2017 at 12:18 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
Peter Eisentraut wrote:
Remove objname/objargs split for referring to objects
I don't know if this is the guilty commit, but I'm now getting these
compiler warnings:/pgsql/source/master/src/backend/catalog/objectaddress.c: In function 'get_object_address':
/pgsql/source/master/src/backend/catalog/objectaddress.c:1624:33: warning: 'typenames[1]' may be used uninitialized in this function [-Wmaybe-uninitialized]
ereport(ERROR,
^
/pgsql/source/master/src/backend/catalog/objectaddress.c:1578:8: note: 'typenames[1]' was declared here
char *typenames[2];
^
/pgsql/source/master/src/backend/catalog/objectaddress.c:1624:33: warning: 'typenames[0]' may be used uninitialized in this function [-Wmaybe-uninitialized]
ereport(ERROR,
^
/pgsql/source/master/src/backend/catalog/objectaddress.c:1578:8: note: 'typenames[0]' was declared here
char *typenames[2];
^
What are you using as CFLAGS? As both typenames should be normally
set, what about initializing those fields with NULL and add an
assertion like the attached?
--
Michael
Attachments:
objaddr-warnings.patchtext/x-patch; charset=US-ASCII; name=objaddr-warnings.patchDownload
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 3a7f049247..52df94439d 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -1552,7 +1552,7 @@ get_object_address_opf_member(ObjectType objtype,
ObjectAddress address;
ListCell *cell;
List *copy;
- char *typenames[2];
+ char *typenames[2] = {NULL, NULL};
Oid typeoids[2];
int membernum;
int i;
@@ -1581,6 +1581,8 @@ get_object_address_opf_member(ObjectType objtype,
break;
}
+ Assert(typenames[0] != NULL && typenames[1] != NULL);
+
switch (objtype)
{
case OBJECT_AMOP:
Michael Paquier wrote:
What are you using as CFLAGS? As both typenames should be normally
set, what about initializing those fields with NULL and add an
assertion like the attached?
Actually, my compiler was right -- this was an ancient bug I introduced
in 9.5 (commit a61fd533), and this new warning was my compiler being a
bit smarter now for some reason. The problem is we were trying to
extract String value from a TypeName node, which obviously doesn't work
very well.
I pushed a real fix, not just a compiler-silencer, along with a few
lines in object_address.sql to make sure it works properly. Maybe we
need a few more tests cases for other parts of pg_get_object_address.
Pushed fix, backpatched to 9.5.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 3/16/17 11:56, Alvaro Herrera wrote:
Michael Paquier wrote:
What are you using as CFLAGS? As both typenames should be normally
set, what about initializing those fields with NULL and add an
assertion like the attached?Actually, my compiler was right -- this was an ancient bug I introduced
in 9.5 (commit a61fd533), and this new warning was my compiler being a
bit smarter now for some reason. The problem is we were trying to
extract String value from a TypeName node, which obviously doesn't work
very well.I pushed a real fix, not just a compiler-silencer, along with a few
lines in object_address.sql to make sure it works properly. Maybe we
need a few more tests cases for other parts of pg_get_object_address.Pushed fix, backpatched to 9.5.
I am now seeing the warnings that Michael was reporting *after* your
commit in 9.5 and 9.6.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Peter Eisentraut wrote:
On 3/16/17 11:56, Alvaro Herrera wrote:
Michael Paquier wrote:
What are you using as CFLAGS? As both typenames should be normally
set, what about initializing those fields with NULL and add an
assertion like the attached?Actually, my compiler was right -- this was an ancient bug I introduced
in 9.5 (commit a61fd533), and this new warning was my compiler being a
bit smarter now for some reason. The problem is we were trying to
extract String value from a TypeName node, which obviously doesn't work
very well.I pushed a real fix, not just a compiler-silencer, along with a few
lines in object_address.sql to make sure it works properly. Maybe we
need a few more tests cases for other parts of pg_get_object_address.Pushed fix, backpatched to 9.5.
I am now seeing the warnings that Michael was reporting *after* your
commit in 9.5 and 9.6.
Do you mean the ones I reported?
I don't get anything with my compiler (gcc (Debian 4.9.2-10) 4.9.2) and
I don't see anything in the few logs I looked at (clang, 9.6) from
buildfarm either. What are you seeing specifically?
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 3/17/17 11:06, Alvaro Herrera wrote:
I don't get anything with my compiler (gcc (Debian 4.9.2-10) 4.9.2) and
I don't see anything in the few logs I looked at (clang, 9.6) from
buildfarm either. What are you seeing specifically?
cc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute
-Wformat-security -fno-strict-aliasing -fwrapv
-fexcess-precision=standard -g -O2 -I../../../src/include
-D_FORTIFY_SOURCE=2 -DLINUX_OOM_ADJ -D_GNU_SOURCE -I/usr/include/libxml2
-c -o objectaddress.o objectaddress.c
objectaddress.c: In function �get_object_address�:
objectaddress.c:1650:24: warning: �typeoids[1]� may be used
uninitialized in this function [-Wmaybe-uninitialized]
objectaddress.c:1582:6: note: �typeoids[1]� was declared here
objectaddress.c:1627:43: warning: �typenames[1]� may be used
uninitialized in this function [-Wmaybe-uninitialized]
objectaddress.c:1581:12: note: �typenames[1]� was declared here
objectaddress.c:1650:24: warning: �typeoids[0]� may be used
uninitialized in this function [-Wmaybe-uninitialized]
objectaddress.c:1582:6: note: �typeoids[0]� was declared here
objectaddress.c:1627:43: warning: �typenames[0]� may be used
uninitialized in this function [-Wmaybe-uninitialized]
objectaddress.c:1581:12: note: �typenames[0]� was declared here
cc (Debian 4.7.2-5) 4.7.2
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Peter Eisentraut wrote:
cc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute
-Wformat-security -fno-strict-aliasing -fwrapv
-fexcess-precision=standard -g -O2 -I../../../src/include
-D_FORTIFY_SOURCE=2 -DLINUX_OOM_ADJ -D_GNU_SOURCE -I/usr/include/libxml2
-c -o objectaddress.o objectaddress.c
objectaddress.c: In function ‘get_object_address’:
objectaddress.c:1650:24: warning: ‘typeoids[1]’ may be used
uninitialized in this function [-Wmaybe-uninitialized]
objectaddress.c:1582:6: note: ‘typeoids[1]’ was declared here
objectaddress.c:1627:43: warning: ‘typenames[1]’ may be used
uninitialized in this function [-Wmaybe-uninitialized]
objectaddress.c:1581:12: note: ‘typenames[1]’ was declared here
objectaddress.c:1650:24: warning: ‘typeoids[0]’ may be used
uninitialized in this function [-Wmaybe-uninitialized]
objectaddress.c:1582:6: note: ‘typeoids[0]’ was declared here
objectaddress.c:1627:43: warning: ‘typenames[0]’ may be used
uninitialized in this function [-Wmaybe-uninitialized]
objectaddress.c:1581:12: note: ‘typenames[0]’ was declared herecc (Debian 4.7.2-5) 4.7.2
Oh yeah, buildfarm member sitella shows that. Does the attached patch
fix it? (pretty much the same thing Michael attached, but we also need
to handle typeoids)
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
objaddr-warn.patchtext/plain; charset=us-asciiDownload
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 8ccc171..7f73fc0 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -1578,8 +1578,8 @@ get_object_address_opf_member(ObjectType objtype,
ObjectAddress address;
ListCell *cell;
List *copy;
- TypeName *typenames[2];
- Oid typeoids[2];
+ TypeName *typenames[2] = { NULL, NULL };
+ Oid typeoids[2] = { InvalidOid, InvalidOid };
int membernum;
int i;
@@ -1607,6 +1607,9 @@ get_object_address_opf_member(ObjectType objtype,
break;
}
+ Assert(typenames[0] != NULL && typenames[1] != NULL);
+ Assert(typeoids[0] != InvalidOid && typeoids[1] != InvalidOid);
+
switch (objtype)
{
case OBJECT_AMOP:
On 3/17/17 13:39, Alvaro Herrera wrote:
Oh yeah, buildfarm member sitella shows that. Does the attached patch
fix it? (pretty much the same thing Michael attached, but we also need
to handle typeoids)
Yes, that makes the warning go away.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers