Re: [HACKERS] replicating DROP commands across servers
Jeff Janes wrote: > On Fri, Jan 2, 2015 at 9:59 PM, Jeff Janes wrote: > > This commit 3f88672a4e4d8e648d24ccc65 seems to have broken pg_upgrade for > > pg_trgm. It seems I failed to notice the get_object_address in the ALTER EXTENSION path. Should be fixed now. I looked for similar missed callsites and didn't find anything. Thanks for the report! -- Álvaro Herrerahttp://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
Re: [HACKERS] replicating DROP commands across servers
On Fri, Jan 2, 2015 at 9:59 PM, Jeff Janes wrote:
> On Mon, Dec 29, 2014 at 2:15 PM, Alvaro Herrera
> wrote:
>
>> Here's a patch that tweaks the grammar to use TypeName in COMMENT,
>> SECURITY LABEL, and DROP for the type and domain cases. The required
>> changes in the code are pretty minimal, thankfully. Note the slight
>> changes to the new object_address test also.
>>
>> I think I'm pretty much done with this now, so I intend to push this
>> first thing tomorrow and then the rebased getObjectIdentityParts patch
>> sometime later.
>>
>
>
> This commit 3f88672a4e4d8e648d24ccc65 seems to have broken pg_upgrade for
> pg_trgm.
>
> On 9.2.9 freshly initdb and started with default config:
>
> $ createdb jjanes
>
> in psql:
>
> create extension pg_trgm ;
> create table foo (x text);
> create index on foo using gin (upper(x) gin_trgm_ops);
>
> Then run 9.5's pg_upgrade and it fails at the stage of restoring the
> database schema.
>
>
The problem also occurs doing a self-upgrade from 9.5 to 9.5.
The contents of the dump not changed meaningfully between 9.4 and 9.5.
I've isolated the problem to the backend applying the pg_restore of the
dump, regardless of which version created the dump.
After compiling 3c9e4cdbf2ec876dbb7 with CFLAGS="-fno-omit-frame-pointer",
I get this backtrace for the core-dump of postmaster during the pg_restore:
Core was generated by `postgres: jjanes jjanes [local] ALTER EXTENSION
'.
Program terminated with signal 11, Segmentation fault.
#0 0x005135ff in NameListToString (names=0x257fcf8) at
namespace.c:2935
2935if (IsA(name, String))
(gdb) bt
#0 0x005135ff in NameListToString (names=0x257fcf8) at
namespace.c:2935
#1 0x00512f33 in DeconstructQualifiedName (names=0x257fcf8,
nspname_p=0x7fff419bc960, objname_p=0x7fff419bc958) at namespace.c:2648
#2 0x0058a746 in LookupTypeName (pstate=0x0, typeName=0x257fd10,
typmod_p=0x0, missing_ok=0 '\000') at parse_type.c:153
#3 0x005220b4 in get_object_address_type (objtype=OBJECT_TYPE,
objname=0x257fd50, missing_ok=0 '\000') at objectaddress.c:1306
#4 0x00520cf5 in get_object_address (objtype=OBJECT_TYPE,
objname=0x257fd50, objargs=0x0, relp=0x7fff419bcb58, lockmode=4,
missing_ok=0 '\000')
at objectaddress.c:678
#5 0x005c0f36 in ExecAlterExtensionContentsStmt (stmt=0x257fd80)
at extension.c:2906
#6 0x0077508c in ProcessUtilitySlow (parsetree=0x257fd80,
queryString=0x254f990 "\n-- For binary upgrade, must preserve pg_type
oid\nSELECT
binary_upgrade.set_next_pg_type_oid('16394'::pg_catalog.oid);\n\n\n-- For
binary upgrade, must preserve pg_type array oid\nSELECT binary_upgrade.se"...,
context=PROCESS_UTILITY_QUERY, params=0x0, dest=0x2581b60,
completionTag=0x7fff419bd100 "") at utility.c:1167
#7 0x0077490e in standard_ProcessUtility (parsetree=0x257fd80,
queryString=0x254f990 "\n-- For binary upgrade, must preserve pg_type
oid\nSELECT
binary_upgrade.set_next_pg_type_oid('16394'::pg_catalog.oid);\n\n\n-- For
binary upgrade, must preserve pg_type array oid\nSELECT binary_upgrade.se"...,
context=PROCESS_UTILITY_QUERY, params=0x0, dest=0x2581b60,
completionTag=0x7fff419bd100 "") at utility.c:840
#8 0x00773bcc in ProcessUtility (parsetree=0x257fd80,
queryString=0x254f990 "\n-- For binary upgrade, must preserve pg_type
oid\nSELECT
binary_upgrade.set_next_pg_type_oid('16394'::pg_catalog.oid);\n\n\n-- For
binary upgrade, must preserve pg_type array oid\nSELECT binary_upgrade.se"...,
context=PROCESS_UTILITY_QUERY, params=0x0, dest=0x2581b60,
completionTag=0x7fff419bd100 "") at utility.c:313
#9 0x00772dd6 in PortalRunUtility (portal=0x2505b90,
utilityStmt=0x257fd80, isTopLevel=0 '\000', dest=0x2581b60,
completionTag=0x7fff419bd100 "")
at pquery.c:1187
#10 0x00772f8c in PortalRunMulti (portal=0x2505b90, isTopLevel=0
'\000', dest=0x2581b60, altdest=0x2581b60, completionTag=0x7fff419bd100 "")
at pquery.c:1318
#11 0x00772560 in PortalRun (portal=0x2505b90,
count=9223372036854775807, isTopLevel=0 '\000', dest=0x2581b60,
altdest=0x2581b60,
completionTag=0x7fff419bd100 "") at pquery.c:816
#12 0x0076c868 in exec_simple_query (
query_string=0x254f990 "\n-- For binary upgrade, must preserve pg_type
oid\nSELECT
binary_upgrade.set_next_pg_type_oid('16394'::pg_catalog.oid);\n\n\n-- For
binary upgrade, must preserve pg_type array oid\nSELECT binary_upgrade.se"...)
at postgres.c:1045
#13 0x007708a2 in PostgresMain (argc=1, argv=0x24ed5e0,
dbname=0x24ed4b8 "jjanes", username=0x24ed4a0 "jjanes") at postgres.c:4012
#14 0x00701940 in BackendRun (port=0x250c1b0) at postmaster.c:4130
#15 0x00701083 in BackendStartup (port=0x250c1b0) at
postmaster.c:3805
#16 0x006fd8c5 in ServerLoop () at postmaster.c:1573
#17 0x006fd013 in PostmasterMain (argc=18, argv=0x24ec480) at
postmaster.c:1220
#18 0x0066476b in main (argc=18,
Re: [HACKERS] replicating DROP commands across servers
On Mon, Dec 29, 2014 at 2:15 PM, Alvaro Herrera wrote: > Here's a patch that tweaks the grammar to use TypeName in COMMENT, > SECURITY LABEL, and DROP for the type and domain cases. The required > changes in the code are pretty minimal, thankfully. Note the slight > changes to the new object_address test also. > > I think I'm pretty much done with this now, so I intend to push this > first thing tomorrow and then the rebased getObjectIdentityParts patch > sometime later. > This commit 3f88672a4e4d8e648d24ccc65 seems to have broken pg_upgrade for pg_trgm. On 9.2.9 freshly initdb and started with default config: $ createdb jjanes in psql: create extension pg_trgm ; create table foo (x text); create index on foo using gin (upper(x) gin_trgm_ops); Then run 9.5's pg_upgrade and it fails at the stage of restoring the database schema. The relevant log files are attached Cheers, Jeff pg_upgrade_server.log Description: Binary data pg_upgrade_dump_16384.log Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] replicating DROP commands across servers
Here's a patch that tweaks the grammar to use TypeName in COMMENT,
SECURITY LABEL, and DROP for the type and domain cases. The required
changes in the code are pretty minimal, thankfully. Note the slight
changes to the new object_address test also.
I think I'm pretty much done with this now, so I intend to push this
first thing tomorrow and then the rebased getObjectIdentityParts patch
sometime later.
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
commit 701a2b7cc3cc6dff865db64ecaeb2fd8bb07b396
Author: Alvaro Herrera
Date: Mon Dec 29 18:55:59 2014 -0300
Use TypeName to represent type names in certain commands
In COMMENT, DROP and SECURITY LABEL we were representing types as a list
of names, rather than the TypeName struct that's used everywhere; this
practice also recently found its way into the new pg_get_object_address.
Right now it doesn't affect anything (other than some code cleanliness),
but it is more problematic for future changes that operate with object
addresses from the SQL interface; type details such as array-ness were
being forgotten.
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 7cb46e1..9ca609d 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -646,13 +646,11 @@ get_object_address(ObjectType objtype, List *objname, List *objargs,
break;
case OBJECT_DOMCONSTRAINT:
{
- List *domname;
ObjectAddress domaddr;
char *constrname;
- domname = list_truncate(list_copy(objname), list_length(objname) - 1);
- constrname = strVal(llast(objname));
- domaddr = get_object_address_type(OBJECT_DOMAIN, domname, missing_ok);
+ domaddr = get_object_address_type(OBJECT_DOMAIN, objname, missing_ok);
+ constrname = strVal(linitial(objargs));
address.classId = ConstraintRelationId;
address.objectId = get_domain_constraint_oid(domaddr.objectId,
@@ -1291,14 +1289,13 @@ get_object_address_attrdef(ObjectType objtype, List *objname,
* Find the ObjectAddress for a type or domain
*/
static ObjectAddress
-get_object_address_type(ObjectType objtype,
- List *objname, bool missing_ok)
+get_object_address_type(ObjectType objtype, List *objname, bool missing_ok)
{
ObjectAddress address;
TypeName *typename;
Type tup;
- typename = makeTypeNameFromNameList(objname);
+ typename = (TypeName *) linitial(objname);
address.classId = TypeRelationId;
address.objectId = InvalidOid;
@@ -1428,27 +1425,8 @@ pg_get_object_address(PG_FUNCTION_ARGS)
* given object type. Most use a simple string Values list, but there
* are some exceptions.
*/
- if (type == OBJECT_TYPE || type == OBJECT_DOMAIN)
- {
- Datum *elems;
- bool *nulls;
- int nelems;
- TypeName *typname;
-
- deconstruct_array(namearr, TEXTOID, -1, false, 'i',
- &elems, &nulls, &nelems);
- if (nelems != 1)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("name list length must be exactly %d", 1)));
- if (nulls[0])
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("name or argument lists may not contain nulls")));
- typname = typeStringToTypeName(TextDatumGetCString(elems[0]));
- name = typname->names;
- }
- else if (type == OBJECT_CAST)
+ if (type == OBJECT_TYPE || type == OBJECT_DOMAIN || type == OBJECT_CAST ||
+ type == OBJECT_DOMCONSTRAINT)
{
Datum *elems;
bool *nulls;
@@ -1533,18 +1511,13 @@ pg_get_object_address(PG_FUNCTION_ARGS)
*/
switch (type)
{
- case OBJECT_DOMCONSTRAINT:
- if (list_length(name) < 2)
-ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("name list length must be at least %d", 2)));
- break;
case OBJECT_LARGEOBJECT:
if (list_length(name) != 1)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("name list length must be %d", 1)));
+ errmsg("name list length must be exactly %d", 1)));
break;
+ case OBJECT_DOMCONSTRAINT:
case OBJECT_OPCLASS:
case OBJECT_OPFAMILY:
case OBJECT_CAST:
diff --git a/src/backend/commands/dropcmds.c b/src/backend/commands/dropcmds.c
index 8583581..21a2ae4 100644
--- a/src/backend/commands/dropcmds.c
+++ b/src/backend/commands/dropcmds.c
@@ -264,10 +264,14 @@ does_not_exist_skipping(ObjectType objtype, List *objname, List *objargs)
{
case OBJECT_TYPE:
case OBJECT_DOMAIN:
- if (!schema_does_not_exist_skipping(objname, &msg, &name))
{
-msg = gettext_noop("type \"%s\" does not exist, skipping");
-name = TypeNameToString(makeTypeNameFromNameList(objname));
+TypeName *typ = linitial(objname);
+
+if (!schema_does_not_exist_skipping(typ->names, &msg, &name))
+{
+ msg = gettext_noop("type \"%s\" does not exist, skipping");
+ name = TypeNameToString(typ);
+}
}
break;
c
Re: [HACKERS] replicating DROP commands across servers
(The changes in the regression test are bogus, BTW; I didn't care enough
to get them fixed before sorting out the rest of the mess.)
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
commit 89c8cbed0072ad4d921128b834fcb4f9e2eb4c33
Author: Alvaro Herrera
Date: Mon Dec 22 18:32:43 2014 -0300
array objname/objargs stuff
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 24c64b7..112b6a0 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -17772,6 +17772,23 @@ FOR EACH ROW EXECUTE PROCEDURE suppress_redundant_updates_trigger();
identifier present in the identity is quoted if necessary.
+
+address_names
+text[]
+
+ An array that, together with address_args,
+ can be used by the pg_get_object_address() to
+ recreate the object address in a remote server containing an
+ identically named object of the same kind.
+
+
+
+address_args
+text[]
+
+ Complement for address_names above.
+
+
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 85079d6..789af5f 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -74,6 +74,7 @@
#include "utils/builtins.h"
#include "utils/fmgroids.h"
#include "utils/lsyscache.h"
+#include "utils/memutils.h"
#include "utils/syscache.h"
#include "utils/tqual.h"
@@ -556,8 +557,9 @@ static void getRelationTypeDescription(StringInfo buffer, Oid relid,
int32 objectSubId);
static void getProcedureTypeDescription(StringInfo buffer, Oid procid);
static void getConstraintTypeDescription(StringInfo buffer, Oid constroid);
-static void getOpFamilyIdentity(StringInfo buffer, Oid opfid);
-static void getRelationIdentity(StringInfo buffer, Oid relid);
+static void getOpFamilyIdentity(StringInfo buffer, Oid opfid, List **objname,
+ List **objargs);
+static void getRelationIdentity(StringInfo buffer, Oid relid, List **objname);
/*
* Translate an object name and arguments (as passed by the parser) to an
@@ -2960,6 +2962,66 @@ pg_identify_object(PG_FUNCTION_ARGS)
}
/*
+ * SQL-level callable function to obtain object type + identity
+ */
+Datum
+pg_identify_object_as_address(PG_FUNCTION_ARGS)
+{
+ Oid classid = PG_GETARG_OID(0);
+ Oid objid = PG_GETARG_OID(1);
+ int32 subobjid = PG_GETARG_INT32(2);
+ ObjectAddress address;
+ char *identity;
+ List *names;
+ List *args;
+ Datum values[3];
+ bool nulls[3];
+ TupleDesc tupdesc;
+ HeapTuple htup;
+
+ address.classId = classid;
+ address.objectId = objid;
+ address.objectSubId = subobjid;
+
+ /*
+ * Construct a tuple descriptor for the result row. This must match this
+ * function's pg_proc entry!
+ */
+ tupdesc = CreateTemplateTupleDesc(3, false);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 1, "type",
+ TEXTOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 2, "object_names",
+ TEXTARRAYOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 3, "object_args",
+ TEXTARRAYOID, -1, 0);
+
+ tupdesc = BlessTupleDesc(tupdesc);
+
+ /* object type */
+ values[0] = CStringGetTextDatum(getObjectTypeDescription(&address));
+ nulls[0] = false;
+
+ /* object identity */
+ identity = getObjectIdentityParts(&address, &names, &args);
+ pfree(identity);
+
+ /* object_names */
+ values[1] = PointerGetDatum(strlist_to_textarray(names));
+ nulls[1] = false;
+
+ /* object_args */
+ if (args)
+ values[2] = PointerGetDatum(strlist_to_textarray(args));
+ else
+ values[2] = PointerGetDatum(construct_empty_array(TEXTOID));
+ nulls[2] = false;
+
+ htup = heap_form_tuple(tupdesc, values, nulls);
+
+ PG_RETURN_DATUM(HeapTupleGetDatum(htup));
+}
+
+/*
* Return a palloc'ed string that describes the type of object that the
* passed address is for.
*
@@ -3215,7 +3277,7 @@ getProcedureTypeDescription(StringInfo buffer, Oid procid)
}
/*
- * Return a palloc'ed string that identifies an object.
+ * Obtain a given object's identity, as a palloc'ed string.
*
* This is for machine consumption, so it's not translated. All elements are
* schema-qualified when appropriate.
@@ -3223,14 +3285,42 @@ getProcedureTypeDescription(StringInfo buffer, Oid procid)
char *
getObjectIdentity(const ObjectAddress *object)
{
+ return getObjectIdentityParts(object, NULL, NULL);
+}
+
+/*
+ * As above, but more detailed.
+ *
+ * There are two sets of return values: the identity itself as a palloc'd
+ * string is returned. objname and objargs, if not NULL, are output parameters
+ * that receive lists of C-strings that are useful to give back to
+ * get_object_address() to reconstruct the ObjectAddress.
+ */
+char *
+getObjectIdentityParts(const ObjectAddress *object,
+ List **objname, List **objargs)
+{
StringInfoData buffer;
initStrin
Re: [HACKERS] replicating DROP commands across servers
Alvaro Herrera wrote:
> Patch 0005 adds getObjectIdentityParts(), which returns the object
> identity in arrays that can be passed to pg_get_object_address. This
> part needs slight revisions so I'm not sure I will be able to push
> tomorrow.
Here's a fresh version of this patch. I chose to add a SQL-accessible
version, pg_identify_object_as_address, to make it easier to test. In
doing so I noticed a couple of bugs, and most interestingly I noticed
that it was essentially impossible to cleanly address an array type;
doing a roundtrip through the new functions would get me the base type
when I used "integer[]" but the array type when I used "_int4". This
looked like a problem, so I traced through it and noticed that we're
using the type name *list* as a list, rather than as a TypeName, to
refer to OBJECT_TYPE and OBJECT_DOMAIN; I hadn't understood the
significance of this until I realized that domains would be represented
with arrayBounds set to a non-empty list for the integer[] syntax, but
the name list would have "pg_catalog integer" only; when the rest of
TypeName was discarded, the fact that we were talking about an array was
completely forgotten. Before the dawn of time we had this:
-static void
-CommentType(List *typename, char *comment)
-{
- TypeName *tname;
- Oid oid;
-
- /* XXX a bit of a crock; should accept TypeName in COMMENT syntax */
- tname = makeTypeNameFromNameList(typename);
where the XXX comment was removed by commit c10575ff005c330d047534562
without a corresponding comment in the new function.
I'm going to see about changing the grammar to get this fixed; this
patch is important because it will enable us to complete^Wcontinue
working on the DDL deparse testing framework.
--
Álvaro Herrerahttp://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
Re: [HACKERS] replicating DROP commands across servers
On 25 December 2014 at 04:47, Tom Lane wrote: > David Rowley writes: > > On 25 December 2014 at 00:34, Andres Freund > wrote: > >> I really wonder if we can't make msvc reliably recognize this kind of > >> scenario - especially this case is pretty trivial? > > > The attached patch removes the warning, but likely can't be used in case > > someone somewhere is doing elog(var++, "my error"); > > Yeah, we're *not* doing that. There are definitely places where > ereport/elog are used with nonconstant elevel. > > Agreed. The patch was intended as a demo of where the problem is. Although I don't see why non-const elevel matters. Non-stable expressions are what actually matter. > It's curious though that MSVC fails to notice that the variable never > changes. I wonder whether we could get away with changing the elog > macro to do > const int elevel_ = (elevel); > as ereport does, and whether it would help if so. > > Unlikely, as the one that was just fixed above is an ereport. I'll dig around a little more and see if there's some way to get MSVC to optimise this somehow. The 1% reduction in the postgres.exe seems worth a little bit of investigation time. Regards David Rowley
Re: [HACKERS] replicating DROP commands across servers
David Rowley writes: > On 25 December 2014 at 00:34, Andres Freund wrote: >> I really wonder if we can't make msvc reliably recognize this kind of >> scenario - especially this case is pretty trivial? > The attached patch removes the warning, but likely can't be used in case > someone somewhere is doing elog(var++, "my error"); Yeah, we're *not* doing that. There are definitely places where ereport/elog are used with nonconstant elevel. It's curious though that MSVC fails to notice that the variable never changes. I wonder whether we could get away with changing the elog macro to do const int elevel_ = (elevel); as ereport does, and whether it would help if so. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] replicating DROP commands across servers
Andres Freund writes: > I really wonder if we can't make msvc reliably recognize this kind of > scenario - especially this case is pretty trivial? Even if MSVC did understand pg_unreachable(), there would be other compilers that didn't, so we'd need to worry about suppressing such warnings anyhow. Personally I'm just as happy that we have instances of this case in the buildfarm where we can check it easily. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] replicating DROP commands across servers
On 25 December 2014 at 00:34, Andres Freund wrote:
> On 2014-12-24 21:54:20 +1300, David Rowley wrote:
> > On 24 December 2014 at 07:41, Alvaro Herrera
> > wrote:
> >
> > > I have pushed patches 0001 through 0004, with some revisions. Only the
> > > final part is missing.
> > >
> > >
> > Hi Alvaro,
> >
> > Would you be able to commit the attached? It just fixes a new compiler
> > warning that I'm seeing on MSVC.
> >
> > src\backend\parser\parse_type.c(795): warning C4715:
> 'typeStringToTypeName'
> > : not all control paths return a value [D:\Postgres\a\postgres.vcxproj]
>
> Pushed.
>
> Thanks
> I really wonder if we can't make msvc reliably recognize this kind of
> scenario - especially this case is pretty trivial?
>
> Which of:
> #if defined(HAVE__BUILTIN_UNREACHABLE) && !defined(USE_ASSERT_CHECKING)
> #define pg_unreachable() __builtin_unreachable()
> #elif defined(_MSC_VER) && !defined(USE_ASSERT_CHECKING)
> #define pg_unreachable() __assume(0)
> #else
> #define pg_unreachable() abort()
> #endif
>
>
I don't think the problem is here. The problem is the the elevel being set
to a variable in the elog macro to prevent the multiple evaluation problem,
then since it does int elevel_ = elevel ... if (elevel_ >= ERROR) that's
not constant, or at least the microsoft compiler is not smart enough to see
that it is.
The attached patch removes the warning, but likely can't be used in case
someone somewhere is doing elog(var++, "my error");
Compiling with the attached shaves almost 1% off the size of postgres.exe:
before; 5,882,368 bytes
after: 5,830,656 bytes
I've been trawling around to try to see if anything
like __builtin_constant_p() exists for MSVC, but so far I've not found
anything useful.
Kind Regards
David Rowley
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 87438b8..8be68dd 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -121,10 +121,9 @@
#else /*
!HAVE__BUILTIN_CONSTANT_P */
#define ereport_domain(elevel, domain, rest) \
do { \
- const int elevel_ = (elevel); \
- if (errstart(elevel_, __FILE__, __LINE__, PG_FUNCNAME_MACRO,
domain)) \
+ if (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO,
domain)) \
errfinish rest; \
- if (elevel_ >= ERROR) \
+ if (elevel >= ERROR) \
pg_unreachable(); \
} while(0)
#endif /* HAVE__BUILTIN_CONSTANT_P */
@@ -258,12 +257,10 @@ extern intgetinternalerrposition(void);
#else /*
!HAVE__BUILTIN_CONSTANT_P */
#define elog(elevel, ...) \
do { \
- int elevel_; \
- elog_start(__FILE__, __LINE__, PG_FUNCNAME_MACRO); \
- elevel_ = (elevel); \
- elog_finish(elevel_, __VA_ARGS__); \
- if (elevel_ >= ERROR) \
- pg_unreachable(); \
+ elog_start(__FILE__, __LINE__, PG_FUNCNAME_MACRO); \
+ elog_finish(elevel, __VA_ARGS__); \
+ if (elevel >= ERROR) \
+ pg_unreachable(); \
} while(0)
#endif /* HAVE__BUILTIN_CONSTANT_P */
#else /* !HAVE__VA_ARGS */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] replicating DROP commands across servers
On 2014-12-24 21:54:20 +1300, David Rowley wrote: > On 24 December 2014 at 07:41, Alvaro Herrera > wrote: > > > I have pushed patches 0001 through 0004, with some revisions. Only the > > final part is missing. > > > > > Hi Alvaro, > > Would you be able to commit the attached? It just fixes a new compiler > warning that I'm seeing on MSVC. > > src\backend\parser\parse_type.c(795): warning C4715: 'typeStringToTypeName' > : not all control paths return a value [D:\Postgres\a\postgres.vcxproj] Pushed. I really wonder if we can't make msvc reliably recognize this kind of scenario - especially this case is pretty trivial? Which of: #if defined(HAVE__BUILTIN_UNREACHABLE) && !defined(USE_ASSERT_CHECKING) #define pg_unreachable() __builtin_unreachable() #elif defined(_MSC_VER) && !defined(USE_ASSERT_CHECKING) #define pg_unreachable() __assume(0) #else #define pg_unreachable() abort() #endif does your build end up using? Does changing things around make it recognize this and similar cases? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, 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
Re: [HACKERS] replicating DROP commands across servers
On 24 December 2014 at 07:41, Alvaro Herrera
wrote:
> I have pushed patches 0001 through 0004, with some revisions. Only the
> final part is missing.
>
>
Hi Alvaro,
Would you be able to commit the attached? It just fixes a new compiler
warning that I'm seeing on MSVC.
src\backend\parser\parse_type.c(795): warning C4715: 'typeStringToTypeName'
: not all control paths return a value [D:\Postgres\a\postgres.vcxproj]
Kind Regards
David Rowley
diff --git a/src/backend/parser/parse_type.c b/src/backend/parser/parse_type.c
index 299cc53..4ab7fe5 100644
--- a/src/backend/parser/parse_type.c
+++ b/src/backend/parser/parse_type.c
@@ -792,6 +792,7 @@ fail:
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("invalid type name \"%s\"", str)));
+ return NULL; /* keep compiler quiet */
}
/*
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] replicating DROP commands across servers
I have pushed patches 0001 through 0004, with some revisions. Only the final part is missing. -- Álvaro Herrerahttp://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
Re: [HACKERS] replicating DROP commands across servers
Here's a five-part split of the remaining pieces of this patch.
Patch 0001 is the one I posted in
http://www.postgresql.org/message-id/20141220022308.gy1...@alvh.no-ip.org
which adds support for COMMENT ON CONSTRAINT .. ON DOMAIN. This just
splits OBJECT_CONSTRAINT in OBJECT_TABCONSTRAINT and
OBJECT_DOMCONSTRAINT. It includes \dd support and pg_dump support for
comments on domain constraint comments.
I intend to commit this one first thing tomorrow.
Patch 0002 adds OBJECT_DEFAULT support. This is not needed currently,
so there's no bug being fixed; we just need it if we want to use
get_object_address in a way different from currently.
Patch 0003 adds an (unused) table and routine to map the strings
returned by getObjectTypeDescription into enum ObjectType, for use of
0004. It also splits a part of parseTypeString into a new function
typeStringToTypeName(), for use of 0004.
Patch 0004 adds a SQL-callable interface to get_object_address,
imaginatively called pg_get_object_address; this uses the stuff in patch
0003. It includes a simple regression test. The code that prepares
from text arrays into the appropriate List structure is messy because it
needs to mimic parser output.
I intend to push these three patches as a single commit tomorrow.
Patch 0005 adds getObjectIdentityParts(), which returns the object
identity in arrays that can be passed to pg_get_object_address. This
part needs slight revisions so I'm not sure I will be able to push
tomorrow.
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From f5a324e864baf60df989e313740744732c04404d Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Fri, 19 Dec 2014 17:03:29 -0300
Subject: [PATCH 1/5] Distinguish domain constraint from table constraints
---
doc/src/sgml/ref/comment.sgml | 14 ++
src/backend/catalog/objectaddress.c| 26 ++
src/backend/commands/alter.c | 3 ++-
src/backend/commands/event_trigger.c | 3 ++-
src/backend/commands/tablecmds.c | 2 +-
src/backend/parser/gram.y | 18 +-
src/backend/parser/parse_utilcmd.c | 2 +-
src/backend/tcop/utility.c | 5 ++---
src/bin/pg_dump/pg_dump.c | 17 +
src/bin/psql/describe.c| 27 +--
src/include/nodes/parsenodes.h | 3 ++-
src/test/regress/input/constraints.source | 21 +
src/test/regress/output/constraints.source | 19 +++
13 files changed, 141 insertions(+), 19 deletions(-)
diff --git a/doc/src/sgml/ref/comment.sgml b/doc/src/sgml/ref/comment.sgml
index 36a7312..62e1968 100644
--- a/doc/src/sgml/ref/comment.sgml
+++ b/doc/src/sgml/ref/comment.sgml
@@ -28,6 +28,7 @@ COMMENT ON
COLLATION object_name |
COLUMN relation_name.column_name |
CONSTRAINT constraint_name ON table_name |
+ CONSTRAINT constraint_name ON DOMAIN domain_name |
CONVERSION object_name |
DATABASE object_name |
DOMAIN object_name |
@@ -127,6 +128,18 @@ COMMENT ON
+table_name
+domain_name
+
+
+ When creating a comment on a constraint on a table or a domain, these
+ parameteres specify the name of the table or domain on which the
+ constraint is defined.
+
+
+
+
+
source_type
@@ -266,6 +279,7 @@ COMMENT ON COLLATION "fr_CA" IS 'Canadian French';
COMMENT ON COLUMN my_table.my_column IS 'Employee ID number';
COMMENT ON CONVERSION my_conv IS 'Conversion to UTF8';
COMMENT ON CONSTRAINT bar_col_cons ON bar IS 'Constrains column col';
+COMMENT ON CONSTRAINT dom_col_constr ON DOMAIN dom IS 'Constrains col of domain';
COMMENT ON DATABASE my_database IS 'Development Database';
COMMENT ON DOMAIN my_domain IS 'Email Address Domain';
COMMENT ON EXTENSION hstore IS 'implements the hstore data type';
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index e261307..297deb5 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -530,11 +530,28 @@ get_object_address(ObjectType objtype, List *objname, List *objargs,
break;
case OBJECT_RULE:
case OBJECT_TRIGGER:
- case OBJECT_CONSTRAINT:
+ case OBJECT_TABCONSTRAINT:
case OBJECT_POLICY:
address = get_object_address_relobject(objtype, objname,
&relation, missing_ok);
break;
+ case OBJECT_DOMCONSTRAINT:
+{
+ List *domname;
+ ObjectAddress domaddr;
+ char *constrname;
+
+ domname = list_truncate(list_copy(objname), list_length(objname) - 1);
+ constrname = strVal(llast(objname));
+ domaddr = get_object_address_type(OBJECT_DOMAIN, domname, missing_ok);
+
+ address.classId = ConstraintRelationId;
+ address.objectId = get_domain_constraint_oid(domaddr.objectId,
+
Re: [HACKERS] replicating DROP commands across servers
Alvaro Herrera wrote: > Alvaro Herrera wrote: > > Andres Freund wrote: > > > > > Having reread the patch just now I basically see two things to > > > criticize: > > > a) why isn't this accessible at SQL level? That seems easy to address. > > > b) Arguably some of this could well be done in separate commits. > > > > Fair comments. I will split it up. > > Here's a split version. The last part is still missing some polish -- > in particular handling for OBJECT_POLICY, and the SQL interface which > would also allow us to get something in the regression tests. Pushed patch 1. -- Álvaro Herrerahttp://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
Re: [HACKERS] replicating DROP commands across servers
Michael Paquier wrote: > This patch has had no activity for the last two months, is in "Needs > Review" state and has marked as reviewer Dimitri. As there is no > activity from the reviewer, I am moving that to CF 2014-12 and > removing Dimitri as reviewer. If someone wants to have a look at this > patch, feel free to do so. Dimitri, if you are still planning to look > at it, please re-add your name. FWIW I intend to commit the first patch more or less as-is, and then add a SQL function accessor to get_object_address to the second part and commit that one also. -- Álvaro Herrerahttp://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
Re: [HACKERS] replicating DROP commands across servers
On Thu, Oct 16, 2014 at 7:01 AM, Alvaro Herrera wrote: > Alvaro Herrera wrote: >> Andres Freund wrote: >> >> > Having reread the patch just now I basically see two things to >> > criticize: >> > a) why isn't this accessible at SQL level? That seems easy to address. >> > b) Arguably some of this could well be done in separate commits. >> >> Fair comments. I will split it up. > > Here's a split version. The last part is still missing some polish -- > in particular handling for OBJECT_POLICY, and the SQL interface which > would also allow us to get something in the regression tests. > > > Note: in this patch series you can find the ObjectTypeMap thing that you > thought was obsolete in the DDL deparse patch ... This patch has had no activity for the last two months, is in "Needs Review" state and has marked as reviewer Dimitri. As there is no activity from the reviewer, I am moving that to CF 2014-12 and removing Dimitri as reviewer. If someone wants to have a look at this patch, feel free to do so. Dimitri, if you are still planning to look at it, please re-add your name. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] replicating DROP commands across servers
Alvaro, On Wednesday, October 15, 2014, Alvaro Herrera wrote: > Alvaro Herrera wrote: > > Andres Freund wrote: > > > > > Having reread the patch just now I basically see two things to > > > criticize: > > > a) why isn't this accessible at SQL level? That seems easy to address. > > > b) Arguably some of this could well be done in separate commits. > > > > Fair comments. I will split it up. > > Here's a split version. The last part is still missing some polish -- > in particular handling for OBJECT_POLICY, and the SQL interface which > would also allow us to get something in the regression tests. The OBJECT_POLICY bit is on me to clean up and I'm planning to do so shortly. I agree that we likely want policies for other objects also as a couple people have brought up that idea now and will investigate it. I'm planning to address the copy.c comments first and should have a patch later tonight. Thanks! Stephen
Re: [HACKERS] replicating DROP commands across servers
Alvaro Herrera wrote:
> Andres Freund wrote:
>
> > Having reread the patch just now I basically see two things to
> > criticize:
> > a) why isn't this accessible at SQL level? That seems easy to address.
> > b) Arguably some of this could well be done in separate commits.
>
> Fair comments. I will split it up.
Here's a split version. The last part is still missing some polish --
in particular handling for OBJECT_POLICY, and the SQL interface which
would also allow us to get something in the regression tests.
Note: in this patch series you can find the ObjectTypeMap thing that you
thought was obsolete in the DDL deparse patch ...
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
>From 92816868c1c717519a76a83e65cd0b48e3726fbf Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Fri, 4 Apr 2014 16:05:48 -0300
Subject: [PATCH 1/3] add normal/original flags to
pg_event_trigger_dropped_objects
---
doc/src/sgml/func.sgml | 13 ++
src/backend/catalog/dependency.c| 21 ++-
src/backend/commands/event_trigger.c| 16 +---
src/include/catalog/pg_proc.h | 2 +-
src/include/commands/event_trigger.h| 3 ++-
src/test/regress/expected/event_trigger.out | 40 +
src/test/regress/sql/event_trigger.sql | 30 ++
7 files changed, 114 insertions(+), 11 deletions(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 7e5bcd9..45f3efa 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -17583,6 +17583,19 @@ FOR EACH ROW EXECUTE PROCEDURE suppress_redundant_updates_trigger();
Object sub-id (e.g. attribute number for columns)
+original
+bool
+Flag used to identify the root object of the deletion
+
+
+normal
+bool
+
+ Flag indicating that there's a normal dependency relationship
+ in the dependency graph leading to this object
+
+
+
object_type
text
Type of the object
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 256486c..6485e3d 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -205,16 +205,25 @@ deleteObjectsInList(ObjectAddresses *targetObjects, Relation *depRel,
/*
* Keep track of objects for event triggers, if necessary.
*/
- if (trackDroppedObjectsNeeded())
+ if (trackDroppedObjectsNeeded() && !(flags & PERFORM_DELETION_INTERNAL))
{
for (i = 0; i < targetObjects->numrefs; i++)
{
- ObjectAddress *thisobj = targetObjects->refs + i;
-
- if ((!(flags & PERFORM_DELETION_INTERNAL)) &&
-EventTriggerSupportsObjectClass(getObjectClass(thisobj)))
+ const ObjectAddress *thisobj = &targetObjects->refs[i];
+ const ObjectAddressExtra *extra = &targetObjects->extras[i];
+ bool original = false;
+ bool normal = false;
+
+ if (extra->flags & DEPFLAG_ORIGINAL)
+original = true;
+ if (extra->flags & DEPFLAG_NORMAL)
+normal = true;
+ if (extra->flags & DEPFLAG_REVERSE)
+normal = true;
+
+ if (EventTriggerSupportsObjectClass(getObjectClass(thisobj)))
{
-EventTriggerSQLDropAddObject(thisobj);
+EventTriggerSQLDropAddObject(thisobj, original, normal);
}
}
}
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index 1b8c94b..0bab971 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -112,6 +112,8 @@ typedef struct SQLDropObject
const char *objname;
const char *objidentity;
const char *objecttype;
+ bool original;
+ bool normal;
slist_node next;
} SQLDropObject;
@@ -1105,7 +1107,7 @@ trackDroppedObjectsNeeded(void)
* Register one object as being dropped by the current command.
*/
void
-EventTriggerSQLDropAddObject(ObjectAddress *object)
+EventTriggerSQLDropAddObject(const ObjectAddress *object, bool original, bool normal)
{
SQLDropObject *obj;
MemoryContext oldcxt;
@@ -1124,6 +1126,8 @@ EventTriggerSQLDropAddObject(ObjectAddress *object)
obj = palloc0(sizeof(SQLDropObject));
obj->address = *object;
+ obj->original = original;
+ obj->normal = normal;
/*
* Obtain schema names from the object's catalog tuple, if one exists;
@@ -1251,8 +1255,8 @@ pg_event_trigger_dropped_objects(PG_FUNCTION_ARGS)
{
SQLDropObject *obj;
int i = 0;
- Datum values[7];
- bool nulls[7];
+ Datum values[9];
+ bool nulls[9];
obj = slist_container(SQLDropObject, next, iter.cur);
@@ -1268,6 +1272,12 @@ pg_event_trigger_dropped_objects(PG_FUNCTION_ARGS)
/* objsubid */
values[i++] = Int32GetDatum(obj->address.objectSubId);
+ /* original */
+ values[i++] = BoolGetDatum(obj->original);
+
+ /* normal */
+ values[i++] = BoolGetDatum(obj->normal);
+
/* object_type */
values[i++] = C
Re: [HACKERS] replicating DROP commands across servers
Andres Freund wrote: > Having reread the patch just now I basically see two things to > criticize: > a) why isn't this accessible at SQL level? That seems easy to address. > b) Arguably some of this could well be done in separate commits. Fair comments. I will split it up. FWIW, I spent some time today fighting with this stuff but the OBJECT_POLICY stuff has been causing me some trouble. I'm not sure that what's there currently in get_object_address is completely sane -- for one thing I'm unsure that tables are the only object kind in the system that are subject to policies. In that case, we have a shortage of abstraction here, it seems, which will need some additional fixes. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, 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
Re: [HACKERS] replicating DROP commands across servers
On 2014-10-04 21:12:24 -0400, Robert Haas wrote: > On Fri, Oct 3, 2014 at 4:58 PM, Alvaro Herrera > wrote: > > Robert Haas wrote: > >> I'm not really very convinced that it's a good idea to expose this > >> instead of just figuring out a way to parse the object identity. > > > > That's the first thing I tried. But it's not pretty: you have to > > extract schema names by splitting at a period (and what if a schema name > > contains a period?), > > Please tell me that the literals are escaped if necessary. If so, > this is pretty easy. quote_literal() is not a hard transformation to > reverse, and splitting on a unquoted period is not hard... > > split out on ON for certain object types, > > ...nor is splitting on any other fixed text string, such as " ON ". I'm not following here. Maybe just because I'm misunderstanding your position. The patch imo consists out of the following parts: 1) Addition of dependency information to the dropped object list 2) Actual get_object_address() handling for default values - the current behaviour looks pretty borked to me. 3) The reverse of getObjectTypeDescription() 4) getObjectIdentityParts() - a slightly more detailed version of getObjectIdentity() that requires less string parsing 5) drop even trigger support for a few types. Are you saying you want to add a function to do 4) via parsing inside postgres or are you suggesting to do that in every user of this facility? If the former, why would it be preferrable to do string parsing if we have access to the source data? That part of the patch looks trivial to me? If the latter, I don't see the advantage either - this is complicated enough, why should different users repeat the work? Am I misunderstanding you here? Having reread the patch just now I basically see two things to criticize: a) why isn't this accessible at SQL level? That seems easy to address. b) Arguably some of this could well be done in separate commits. > > It's just not sane to try to parse such text strings. > > But this is a pretty ridiculous argument. We have an existing parser > that does it just fine Which happens to be the part of postgres that's copied most often. So it's certainly not something appearing to be trivial. >,and a special-purpose parser that does just > that (and not all of the other stuff that the main parser does) would > be a great deal simpler. That parser also happens to be far from trivial (if we're talking about parseNameAndArgTypes() - which just solves one of the cases. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, 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
Re: [HACKERS] replicating DROP commands across servers
Robert Haas wrote:
> On Fri, Oct 3, 2014 at 4:58 PM, Alvaro Herrera
> wrote:
> > Robert Haas wrote:
> >> I'm not really very convinced that it's a good idea to expose this
> >> instead of just figuring out a way to parse the object identity.
> >
> > That's the first thing I tried. But it's not pretty: you have to
> > extract schema names by splitting at a period (and what if a schema name
> > contains a period?),
>
> Please tell me that the literals are escaped if necessary. If so,
> this is pretty easy. quote_literal() is not a hard transformation to
> reverse, and splitting on a unquoted period is not hard...
I don't think it is necessary to force parsing strings for something
that can be more conveniently obtained from the get go, just because
we're too lazy to change the existing definition of the function. I'm
not saying it is impossible or extremely hard to parse the strings, but
since we can emit the right format with no extra effort, there doesn't
seem to be any point on doing it that way. It's not like this patch
adds excessive extra complexity to this code, either.
I'd say that the most complex part of this patch is the addition of the
two flag ("normal" and "original") columns, which we would need
regardless of the rest of the patch; these are used to tell whether
there are routes to the given object that have the eponymous flags set
in the dependency graph.
> > It's just not sane to try to parse such text strings.
>
> But this is a pretty ridiculous argument. We have an existing parser
> that does it just fine, and a special-purpose parser that does just
> that (and not all of the other stuff that the main parser does) would
> be a great deal simpler.
We have a main parser because we have no other option --- it's the way
stuff gets into the system in the first place. But in this case, it's
not about accepting communication from the outside world, but emitting
state that is already in the database, in a different format.
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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
Re: [HACKERS] replicating DROP commands across servers
Jim Nasby wrote: > On 10/6/14, 11:24 PM, Robert Haas wrote: > > Offlist. > > >>FWIW, I've run into situations more than once in userspace where I need a > >>way to properly separate schema and object name. Generally I can make do > >>using reg* casts and then hitting catalog tables, but it'd be nice if there > >>was an easier way. > > > >Sure, although I think that's a bit of a separate problem. It's hard > >to iterate through a string a character at a time from the SQL level > >so that you can handle stuff like the quote_literal() rules. If we > >want people to be able to do that easily we need to provide tools to > >handle it. But C is actually quite well-suited to such tasks. > > Yeah, I wouldn't want to attempt this in SQL; I was saying that a > built-in function to do this would be broadly useful, not just for > replicating DROPs. Well, most of what you need is served by pg_identify_object, I think: just grab the OID from the appropriate catalog, and the OID of the catalog itself; that function will give you schema and name, which is what you need. Probably the most difficult part is figuring out which reg* cast you want .. and of course there are also cases where there is no such datatype to cast to in the first place. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, 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
Re: [HACKERS] replicating DROP commands across servers
On 10/6/14, 11:24 PM, Robert Haas wrote: Offlist. FWIW, I've run into situations more than once in userspace where I need a way to properly separate schema and object name. Generally I can make do using reg* casts and then hitting catalog tables, but it'd be nice if there was an easier way. Sure, although I think that's a bit of a separate problem. It's hard to iterate through a string a character at a time from the SQL level so that you can handle stuff like the quote_literal() rules. If we want people to be able to do that easily we need to provide tools to handle it. But C is actually quite well-suited to such tasks. Yeah, I wouldn't want to attempt this in SQL; I was saying that a built-in function to do this would be broadly useful, not just for replicating DROPs. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] replicating DROP commands across servers
On Mon, Oct 6, 2014 at 7:03 PM, Jim Nasby wrote: > On 10/4/14, 8:12 PM, Robert Haas wrote: >>> >It's just not sane to try to parse such text strings. >> >> But this is a pretty ridiculous argument. We have an existing parser >> that does it just fine, and a special-purpose parser that does just >> that (and not all of the other stuff that the main parser does) would >> be a great deal simpler. Maybe there are examples other than the ones >> you listed here that demonstrate that this is actually a hard problem, >> but the fact that you might need to undo quote_literal() or search for >> and split on fixed strings does not. > > > FWIW, I've run into situations more than once in userspace where I need a > way to properly separate schema and object name. Generally I can make do > using reg* casts and then hitting catalog tables, but it'd be nice if there > was an easier way. Sure, although I think that's a bit of a separate problem. It's hard to iterate through a string a character at a time from the SQL level so that you can handle stuff like the quote_literal() rules. If we want people to be able to do that easily we need to provide tools to handle it. But C is actually quite well-suited to such tasks. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] replicating DROP commands across servers
On 10/4/14, 8:12 PM, Robert Haas wrote: >It's just not sane to try to parse such text strings. But this is a pretty ridiculous argument. We have an existing parser that does it just fine, and a special-purpose parser that does just that (and not all of the other stuff that the main parser does) would be a great deal simpler. Maybe there are examples other than the ones you listed here that demonstrate that this is actually a hard problem, but the fact that you might need to undo quote_literal() or search for and split on fixed strings does not. FWIW, I've run into situations more than once in userspace where I need a way to properly separate schema and object name. Generally I can make do using reg* casts and then hitting catalog tables, but it'd be nice if there was an easier way. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] replicating DROP commands across servers
On Fri, Oct 3, 2014 at 4:58 PM, Alvaro Herrera wrote: > Robert Haas wrote: >> I'm not really very convinced that it's a good idea to expose this >> instead of just figuring out a way to parse the object identity. > > That's the first thing I tried. But it's not pretty: you have to > extract schema names by splitting at a period (and what if a schema name > contains a period?), Please tell me that the literals are escaped if necessary. If so, this is pretty easy. quote_literal() is not a hard transformation to reverse, and splitting on a unquoted period is not hard... > split out on ON for certain object types, ...nor is splitting on any other fixed text string, such as " ON ". > figure > out parens and argument types and names for functions and aggregates, > etc. I certainly agree that parsing out parens and argument types and names for functions and aggregates is the hardest part of this, mostly because you can't count a comma to mark the end of one argument and the beginning of the next - you have to account for quoted identifiers, and you might be inside a numeric typemod or similar. > It's just not sane to try to parse such text strings. But this is a pretty ridiculous argument. We have an existing parser that does it just fine, and a special-purpose parser that does just that (and not all of the other stuff that the main parser does) would be a great deal simpler. Maybe there are examples other than the ones you listed here that demonstrate that this is actually a hard problem, but the fact that you might need to undo quote_literal() or search for and split on fixed strings does not. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] replicating DROP commands across servers
On 2014-10-03 14:02:09 -0300, Alvaro Herrera wrote: > Since the patch has had good feedback and no further comments arise, I > can just implement support for those two missing object types and push, > and everybody will be happy. Right? I'd like to see a new version before that out here... I don't think there's fundamental issues, but it's complicated enough to warrant that. And I don't think it has gotten enough detailed review. I hope to be able to give a bit more of that... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, 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
Re: [HACKERS] replicating DROP commands across servers
Robert Haas wrote: > I'm not really very convinced that it's a good idea to expose this > instead of just figuring out a way to parse the object identity. That's the first thing I tried. But it's not pretty: you have to extract schema names by splitting at a period (and what if a schema name contains a period?), split out on ON for certain object types, figure out parens and argument types and names for functions and aggregates, etc. It's just not sane to try to parse such text strings. > But I expect to lose that argument. Good :-) -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, 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
Re: [HACKERS] replicating DROP commands across servers
On Fri, Oct 3, 2014 at 2:33 PM, Alvaro Herrera wrote: > Heikki Linnakangas wrote: >> On 10/03/2014 09:06 PM, Alvaro Herrera wrote: > >> >Well, the return value from get_object_address is an ObjectAddress. >> >It's simple enough to create an SQL wrapper that takes the >> >address_names/address_args arrays and return an ObjectAddress; would >> >this be useful? >> >> An ObjectAddress consists of a classid, objid, and objsubid. >> pg_event_trigger_dropped_objects already returns all of those as >> separate fields. What am I missing? > > Precisely the point is not returning those values, because they are > useless to identify the equivalent object in a remote database. What we > need is the object names and other stuff used to uniquely identify it > "by user-visible name". We transmit those name arrays to a remote > server, then on the remote server we can run get_object_address and get > the ObjectAddress, which has the classid,objid,objsubid values you cite ... > but for the remote server. The object can then be dropped there. > > Initially we thought that we would use the object_identity object for > this (which is why we invented that functionality and added the column > in 9.3), but this turned out not to work very well for unusual object > types; hence this patch. I'm not really very convinced that it's a good idea to expose this instead of just figuring out a way to parse the object identity. But I expect to lose that argument. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] replicating DROP commands across servers
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > Stephen Frost wrote: > > ahh, ok, that makes a bit more sense, sorry for missing it. Still makes > > me wonder why objargs gets special treatment at the top of the function > > and objnames doesn't- seems like both should be initialized either > > before being passed in (and perhaps an Assert to verify that they are), > > or they should both be initialized, but I tend to prefer just Assert'ing > > that they are correct on entry- either both are valid pointers to empty > > lists, or both NULL. > > I guess I could initialize objnames to NIL also. I initialize objargs > because that one is unused for a lot of object types (so I would have to > set it to NIL in cases where it's not used), whereas objnames is always > used and thus we know it's always initialized later. > > Maybe what I need here is just a longer comment explaining this ... A one-line comment that it's always reset below would be sufficient for me. Thanks for explaining it :), Stephen signature.asc Description: Digital signature
Re: [HACKERS] replicating DROP commands across servers
Stephen Frost wrote: > * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > > Right. In the "add to objname" cases, there is already some other > > routine that initialized it previously by filling in some stuff; in the > > case above, this happens in the getRelationIdentity() immediately > > preceding this. > > > > In the other cases we initialize on that spot. > > ahh, ok, that makes a bit more sense, sorry for missing it. Still makes > me wonder why objargs gets special treatment at the top of the function > and objnames doesn't- seems like both should be initialized either > before being passed in (and perhaps an Assert to verify that they are), > or they should both be initialized, but I tend to prefer just Assert'ing > that they are correct on entry- either both are valid pointers to empty > lists, or both NULL. I guess I could initialize objnames to NIL also. I initialize objargs because that one is unused for a lot of object types (so I would have to set it to NIL in cases where it's not used), whereas objnames is always used and thus we know it's always initialized later. Maybe what I need here is just a longer comment explaining this ... -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, 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
Re: [HACKERS] replicating DROP commands across servers
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> Right. In the "add to objname" cases, there is already some other
> routine that initialized it previously by filling in some stuff; in the
> case above, this happens in the getRelationIdentity() immediately
> preceding this.
>
> In the other cases we initialize on that spot.
ahh, ok, that makes a bit more sense, sorry for missing it. Still makes
me wonder why objargs gets special treatment at the top of the function
and objnames doesn't- seems like both should be initialized either
before being passed in (and perhaps an Assert to verify that they are),
or they should both be initialized, but I tend to prefer just Assert'ing
that they are correct on entry- either both are valid pointers to empty
lists, or both NULL.
> > I'm also not a huge fan of the big object_type_map, but I also don't
> > have a better solution.
>
> Agreed. We have the ObjectProperty array also in the same file; it
> kinda looks like there is duplication here, but actually there isn't.
Yeah, I did notice that, and noticed that it's not duplication.
> This whole issue is just fallout from the fact that we have three
> different ways to identify object classes: the ObjectClass enum, the
> ObjectType enum, and the relation OIDs of each catalog (actually a
> fourth one, see below). I don't see any other nice way around this; I
> guess we could try to auto-generate these tables somehow from a master
> text file, or something. Not material for this patch, I think.
Agreed that this patch doesn't need to address it and not sure that a
master text file would actually be an improvement.. I had been thinking
if there was some way to have a single mapping which could be used in
either direction, but I didn't see any sensible way to make that work
given that it's not *quite* the same backwards and forewards.
> Note my DDL deparse patch adds a comment:
>
> +/* XXX merge this with ObjectTypeMap? */
> static event_trigger_support_data event_trigger_support[] = {
Yes, I saw that, and that you added a comment that the new map needs to
be updated when changes are made, which is also good.
> and a late revision to that patch added a new function in
> event_triggers.c (not yet posted I think) due to GRANT having its own
> enum of object types, AclObjectKind.
Yeah. Perhaps one day we'll unify all of these, though I'm not 100%
sure it'd be possible...
Thanks!
Stephen
signature.asc
Description: Digital signature
Re: [HACKERS] replicating DROP commands across servers
Stephen Frost wrote:
> Alvaro,
>
> * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> > + /*
> > +* Make sure that both objname and objargs were passed, or none was.
> > +* Initialize objargs to empty list, which is the most common case.
> > +*/
> > + Assert(PointerIsValid(objname) == PointerIsValid(objargs));
> > + if (objargs)
> > + *objargs = NIL;
> > +
>
> I feel like I must be missing something, but you only explicitly reset
> objargs, not objnames, and then below you sometimes add to objname and
> other times throw away anything which might be there:
>
> > --- 2948,2974
> > attr =
> > get_relid_attribute_name(object->objectId,
> >
> > object->objectSubId);
> > appendStringInfo(&buffer, ".%s",
> > quote_identifier(attr));
> > + if (objname)
> > + *objname = lappend(*objname, attr);
> > }
> > break;
>
> Here's an "add to objname" case, and then:
Right. In the "add to objname" cases, there is already some other
routine that initialized it previously by filling in some stuff; in the
case above, this happens in the getRelationIdentity() immediately
preceding this.
In the other cases we initialize on that spot.
> > --- 3037,3045
> > {
> > appendStringInfo(&buffer, "%s on ",
> >
> > quote_identifier(NameStr(con->conname)));
> > ! getRelationIdentity(&buffer,
> > con->conrelid, objname);
> > ! if (objname)
> > ! *objname = lappend(*objname,
> > pstrdup(NameStr(con->conname)));
> > }
> > else
> > {
>
> And another "add to existing" case.
Note how this one has a getRelationIdentity, just like the first one.
> Guess I have a bit of a hard time with an API that's "we might add to
> this list, or we might replace whatever is there". I think it would be
> best to just initialize both (or assert that they are) and have any
> callers who need to merge the list(s) coming back into an existing list
> handle that themselves.
The thing is, the list is already initialized in all cases to a valid
list in this routine; there is no case that appends to whatever junk
might have been there before this routine started.
> I'm also not a huge fan of the big object_type_map, but I also don't
> have a better solution.
Agreed. We have the ObjectProperty array also in the same file; it
kinda looks like there is duplication here, but actually there isn't.
This whole issue is just fallout from the fact that we have three
different ways to identify object classes: the ObjectClass enum, the
ObjectType enum, and the relation OIDs of each catalog (actually a
fourth one, see below). I don't see any other nice way around this; I
guess we could try to auto-generate these tables somehow from a master
text file, or something. Not material for this patch, I think.
Note my DDL deparse patch adds a comment:
+/* XXX merge this with ObjectTypeMap? */
static event_trigger_support_data event_trigger_support[] = {
and a late revision to that patch added a new function in
event_triggers.c (not yet posted I think) due to GRANT having its own
enum of object types, AclObjectKind.
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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
Re: [HACKERS] replicating DROP commands across servers
Alvaro,
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> + /*
> + * Make sure that both objname and objargs were passed, or none was.
> + * Initialize objargs to empty list, which is the most common case.
> + */
> + Assert(PointerIsValid(objname) == PointerIsValid(objargs));
> + if (objargs)
> + *objargs = NIL;
> +
I feel like I must be missing something, but you only explicitly reset
objargs, not objnames, and then below you sometimes add to objname and
other times throw away anything which might be there:
> --- 2948,2974
> attr =
> get_relid_attribute_name(object->objectId,
>
> object->objectSubId);
> appendStringInfo(&buffer, ".%s",
> quote_identifier(attr));
> + if (objname)
> + *objname = lappend(*objname, attr);
> }
> break;
Here's an "add to objname" case, and then:
> case OCLASS_PROC:
> appendStringInfoString(&buffer,
>
> format_procedure_qualified(object->objectId));
> + if (objname)
> + format_procedure_parts(object->objectId,
> objname, objargs);
> break;
>
> case OCLASS_TYPE:
> ! {
> ! char *typeout;
> !
> ! typeout =
> format_type_be_qualified(object->objectId);
> ! appendStringInfoString(&buffer, typeout);
> ! if (objname)
> ! *objname = list_make1(typeout);
> ! }
> break;
here's a "throw away whatever was in objname" case.
> ***
> *** 2745,2750 getObjectIdentity(const ObjectAddress *object)
> --- 2991,3000
>
> format_type_be_qualified(castForm->castsource),
>
> format_type_be_qualified(castForm->casttarget));
>
> + if (objname)
> + *objname =
> list_make2(format_type_be_qualified(castForm->castsource),
> +
> format_type_be_qualified(castForm->casttarget));
> +
> heap_close(castRel, AccessShareLock);
> break;
> }
And another "throw away" case.
> --- 3037,3045
> {
> appendStringInfo(&buffer, "%s on ",
>
> quote_identifier(NameStr(con->conname)));
> ! getRelationIdentity(&buffer,
> con->conrelid, objname);
> ! if (objname)
> ! *objname = lappend(*objname,
> pstrdup(NameStr(con->conname)));
> }
> else
> {
And another "add to existing" case.
Guess I have a bit of a hard time with an API that's "we might add to
this list, or we might replace whatever is there". I think it would be
best to just initialize both (or assert that they are) and have any
callers who need to merge the list(s) coming back into an existing list
handle that themselves.
I'm also not a huge fan of the big object_type_map, but I also don't
have a better solution.
Thanks,
Stephen
signature.asc
Description: Digital signature
Re: [HACKERS] replicating DROP commands across servers
Alvaro Herrera wrote: > Precisely the point is not returning those values, because they are > useless to identify the equivalent object in a remote database. What we > need is the object names and other stuff used to uniquely identify it > "by user-visible name". We transmit those name arrays to a remote > server, then on the remote server we can run get_object_address and get > the ObjectAddress, which has the classid,objid,objsubid values you cite ... > but for the remote server. The object can then be dropped there. > > Initially we thought that we would use the object_identity object for > this (which is why we invented that functionality and added the column > in 9.3), but this turned out not to work very well for unusual object > types; hence this patch. The other thing to keep in mind is that with all those ObjectAddress thingies you got, you cannot simply construct a DROP command: 1. The objects in the list might be of different types; say a table and a view that are dropped by the same command because of CASCADE. (You could just pass the CASCADE to the other side and hope that it happens to do the same thing; but if the schemas are slightly different, it might not.) 2. DROP OWNED or other commands might have dropped several objects, again of varying types. So what we do in BDR is stuff all those ObjectAddress in an array of them, and then call performMultipleDeletions. There is no way to get this functionality in non-C code; so it's hard to see that it's very useful to have a non-C way to use the original objnames/objargs arrays. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, 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
Re: [HACKERS] replicating DROP commands across servers
Heikki Linnakangas wrote: > On 10/03/2014 09:06 PM, Alvaro Herrera wrote: > >Well, the return value from get_object_address is an ObjectAddress. > >It's simple enough to create an SQL wrapper that takes the > >address_names/address_args arrays and return an ObjectAddress; would > >this be useful? > > An ObjectAddress consists of a classid, objid, and objsubid. > pg_event_trigger_dropped_objects already returns all of those as > separate fields. What am I missing? Precisely the point is not returning those values, because they are useless to identify the equivalent object in a remote database. What we need is the object names and other stuff used to uniquely identify it "by user-visible name". We transmit those name arrays to a remote server, then on the remote server we can run get_object_address and get the ObjectAddress, which has the classid,objid,objsubid values you cite ... but for the remote server. The object can then be dropped there. Initially we thought that we would use the object_identity object for this (which is why we invented that functionality and added the column in 9.3), but this turned out not to work very well for unusual object types; hence this patch. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, 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
Re: [HACKERS] replicating DROP commands across servers
On 10/03/2014 09:06 PM, Alvaro Herrera wrote: Heikki Linnakangas wrote: I had a very brief look at the docs, and these extra outputs from pg_event_trigger_dropped_objects caught my eye: + + address_names + text[] + + An array that, together with address_args, + can be used by the C-language function getObjectAddress() to + recreate the object address in a remote server containing a similar object. + + + + address_args + text[] + + See address_names above. + + I couldn't find a function called getObjectAddress anywhere. Typo? Ah, yeah, it's get_object_address actually. Also, is providing a C-language function the best we can do? The rest of the information returned by pg_event_trigger_dropped_objects is usable from any language. Well, the return value from get_object_address is an ObjectAddress. It's simple enough to create an SQL wrapper that takes the address_names/address_args arrays and return an ObjectAddress; would this be useful? An ObjectAddress consists of a classid, objid, and objsubid. pg_event_trigger_dropped_objects already returns all of those as separate fields. What am I missing? - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] replicating DROP commands across servers
Heikki Linnakangas wrote: > I had a very brief look at the docs, and these extra outputs from > pg_event_trigger_dropped_objects caught my eye: > > >+ > >+ address_names > >+ text[] > >+ > >+ An array that, together with address_args, > >+ can be used by the C-language function getObjectAddress() to > >+ recreate the object address in a remote server containing a > >similar object. > >+ > >+ > >+ > >+ address_args > >+ text[] > >+ > >+ See address_names above. > >+ > >+ > > I couldn't find a function called getObjectAddress anywhere. Typo? Ah, yeah, it's get_object_address actually. > Also, is providing a C-language function the best we can do? The > rest of the information returned by pg_event_trigger_dropped_objects > is usable from any language. Well, the return value from get_object_address is an ObjectAddress. It's simple enough to create an SQL wrapper that takes the address_names/address_args arrays and return an ObjectAddress; would this be useful? -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, 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
Re: [HACKERS] replicating DROP commands across servers
On 10/03/2014 08:08 PM, Stephen Frost wrote: Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: There are three fixmes in the code. One can be handled by just removing the line; we don't really care about duplicating 10 lines of boilerplate code. The other two mean missing support for domain constraints and for default ACLs. Is there absolutely no feedback to be had on the mechanism used by the patch? Since the patch has had good feedback and no further comments arise, I can just implement support for those two missing object types and push, and everybody will be happy. Right? In general, I'd say yes, but I'll take a look at the patch now and provide feedback in a couple hours anyway. Thanks Stephen! I had a very brief look at the docs, and these extra outputs from pg_event_trigger_dropped_objects caught my eye: + + address_names + text[] + + An array that, together with address_args, + can be used by the C-language function getObjectAddress() to + recreate the object address in a remote server containing a similar object. + + + + address_args + text[] + + See address_names above. + + I couldn't find a function called getObjectAddress anywhere. Typo? Also, is providing a C-language function the best we can do? The rest of the information returned by pg_event_trigger_dropped_objects is usable from any language. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] replicating DROP commands across servers
Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > There are three fixmes in the code. One can be handled by just removing > the line; we don't really care about duplicating 10 lines of boilerplate > code. The other two mean missing support for domain constraints and for > default ACLs. Is there absolutely no feedback to be had on the > mechanism used by the patch? > > Since the patch has had good feedback and no further comments arise, I > can just implement support for those two missing object types and push, > and everybody will be happy. Right? In general, I'd say yes, but I'll take a look at the patch now and provide feedback in a couple hours anyway. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] replicating DROP commands across servers
Heikki Linnakangas wrote: > On 09/16/2014 09:09 PM, Brightwell, Adam wrote: > >I have given this patch the following review: > > > >- Apply to current master (77e65bf). -- success > >- check-world. --success > >- multiple FIXME statements still exist -- are there plans to fix these > >items? Can the duplicated code be extracted to a static function? > > Nothing seems to be happening to this, so I'm marking this as > returned with feedback. Meh. There are three fixmes in the code. One can be handled by just removing the line; we don't really care about duplicating 10 lines of boilerplate code. The other two mean missing support for domain constraints and for default ACLs. Is there absolutely no feedback to be had on the mechanism used by the patch? Since the patch has had good feedback and no further comments arise, I can just implement support for those two missing object types and push, and everybody will be happy. Right? -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, 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
Re: [HACKERS] replicating DROP commands across servers
On 09/16/2014 09:09 PM, Brightwell, Adam wrote: I think there's been some changes to this patch since july, care to resend a new version? Sure, here it is. The only difference with the previous version is that it now also supports column defaults. This was found to be a problem when you drop a sequence that some column default depends on -- for example a column declared SERIAL, or a sequence marked with ALTER SEQUENCE OWNED BY. The new code is able to drop both the sequence and the default value (leaving, of course, the rest of the column intact.) This required adding support for such objects in get_object_address. I have given this patch the following review: - Apply to current master (77e65bf). -- success - check-world. --success - multiple FIXME statements still exist -- are there plans to fix these items? Can the duplicated code be extracted to a static function? Nothing seems to be happening to this, so I'm marking this as returned with feedback. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] replicating DROP commands across servers
> > > I think there's been some changes to this patch since july, care to > > resend a new version? > > Sure, here it is. > > The only difference with the previous version is that it now also > supports column defaults. This was found to be a problem when you drop > a sequence that some column default depends on -- for example a column > declared SERIAL, or a sequence marked with ALTER SEQUENCE OWNED BY. The > new code is able to drop both the sequence and the default value > (leaving, of course, the rest of the column intact.) This required > adding support for such objects in get_object_address. I have given this patch the following review: - Apply to current master (77e65bf). -- success - check-world. --success - multiple FIXME statements still exist -- are there plans to fix these items? Can the duplicated code be extracted to a static function? -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] replicating DROP commands across servers
Andres Freund wrote:
> On 2014-06-13 15:50:50 -0400, Alvaro Herrera wrote:
> > Here's a patch implementing the proposed idea. This is used in the
> > Bidirectional Replication stuff by Simon/Andres; it works well.
>
> I think there's been some changes to this patch since july, care to
> resend a new version?
Sure, here it is.
The only difference with the previous version is that it now also
supports column defaults. This was found to be a problem when you drop
a sequence that some column default depends on -- for example a column
declared SERIAL, or a sequence marked with ALTER SEQUENCE OWNED BY. The
new code is able to drop both the sequence and the default value
(leaving, of course, the rest of the column intact.) This required
adding support for such objects in get_object_address.
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***
*** 17564,17569 FOR EACH ROW EXECUTE PROCEDURE suppress_redundant_updates_trigger();
--- 17564,17582
Object sub-id (e.g. attribute number for columns)
+ original
+ bool
+ Flag used to identify the root object of the deletion
+
+
+ normal
+ bool
+
+ Flag indicating that there's a normal dependency relationship
+ in the dependency graph leading to this object
+
+
+
object_type
text
Type of the object
***
*** 17593,17598 FOR EACH ROW EXECUTE PROCEDURE suppress_redundant_updates_trigger();
--- 17606,17627
identifier present in the identity is quoted if necessary.
+
+ address_names
+ text[]
+
+ An array that, together with address_args,
+ can be used by the C-language function getObjectAddress() to
+ recreate the object address in a remote server containing a similar object.
+
+
+
+ address_args
+ text[]
+
+ See address_names above.
+
+
*** a/src/backend/catalog/dependency.c
--- b/src/backend/catalog/dependency.c
***
*** 203,218 deleteObjectsInList(ObjectAddresses *targetObjects, Relation *depRel,
/*
* Keep track of objects for event triggers, if necessary.
*/
! if (trackDroppedObjectsNeeded())
{
for (i = 0; i < targetObjects->numrefs; i++)
{
! ObjectAddress *thisobj = targetObjects->refs + i;
!
! if ((!(flags & PERFORM_DELETION_INTERNAL)) &&
! EventTriggerSupportsObjectClass(getObjectClass(thisobj)))
{
! EventTriggerSQLDropAddObject(thisobj);
}
}
}
--- 203,227
/*
* Keep track of objects for event triggers, if necessary.
*/
! if (trackDroppedObjectsNeeded() && !(flags & PERFORM_DELETION_INTERNAL))
{
for (i = 0; i < targetObjects->numrefs; i++)
{
! const ObjectAddress *thisobj = &targetObjects->refs[i];
! const ObjectAddressExtra *extra = &targetObjects->extras[i];
! bool original = false;
! bool normal = false;
!
! if (extra->flags & DEPFLAG_ORIGINAL)
! original = true;
! if (extra->flags & DEPFLAG_NORMAL)
! normal = true;
! if (extra->flags & DEPFLAG_REVERSE)
! normal = true;
!
! if (EventTriggerSupportsObjectClass(getObjectClass(thisobj)))
{
! EventTriggerSQLDropAddObject(thisobj, original, normal);
}
}
}
*** a/src/backend/catalog/objectaddress.c
--- b/src/backend/catalog/objectaddress.c
***
*** 417,422 static const ObjectPropertyType ObjectProperty[] =
--- 417,513
}
};
+ /*
+ * This struct maps the object types as returned by getObjectTypeDescription
+ * into ObjType enum values. Note that some enum values can be obtained by
+ * different names, and that some string object types do not have corresponding
+ * values in the enum. The user of this map must be careful to test for
+ * invalid values being returned.
+ *
+ * This follows the order of getObjectTypeDescription.
+ */
+ static const struct object_type_map
+ {
+ const char *tm_name;
+ ObjectType tm_type;
+ }
+ ObjectTypeMap[] =
+ {
+ /* OCLASS_CLASS */
+ { "table", OBJECT_TABLE },
+ { "index", OBJECT_INDEX },
+ { "sequence", OBJECT_SEQUENCE },
+ { "toast table", -1 }, /* unmapped */
+ { "view", OBJECT_VIEW },
+ { "materialized view", OBJECT_MATVIEW },
+ { "composite type", OBJECT_COMPOSITE },
+ { "foreign table", OBJECT_FOREIGN_TABLE },
+ { "table column", OBJECT_COLUMN },
+ /* OCLASS_PROC */
+ { "aggregate", OBJECT_AGGREGATE },
+ { "function", OBJECT_FUNCTION },
+ /* OCLASS_TYPE */
+ { "type", OBJECT_TYPE },
+ /* OCLASS_CAST */
+ { "cast", OBJECT_CAST },
+ /* OCLASS_COLLATION */
+ { "collation", OBJECT_COLLATION },
+ /* OCLASS_CONSTRAINT */
+ { "table constraint", OBJECT_
Re: [HACKERS] replicating DROP commands across servers
On 2014-06-13 15:50:50 -0400, Alvaro Herrera wrote: > Here's a patch implementing the proposed idea. This is used in the > Bidirectional Replication stuff by Simon/Andres; it works well. I think there's been some changes to this patch since july, care to resend a new version? I think it's appropriate to mark the patch as "Waiting for Author" instead of "Ready for Committer" till then. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] replicating DROP commands across servers
Hi. I thought I'd review this patch, since pgaudit uses the pg_event_trigger_dropped_objects function. I went through the patch line by line, and I don't really have anything to say about it. I notice that there are some XXX/FIXME comments in the code, but it's not clear if those need to (or can be) fixed before the code is committed. Everything else looks good. I think this is ready for committer. -- Abhijit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] replicating DROP commands across servers
Here's a patch implementing the proposed idea. This is used in the
Bidirectional Replication stuff by Simon/Andres; it works well.
One thing of note is that I added output flags for "normal" and
"original", which mostly come from performDeletion flags. This let one
select only such objects when trying to replicate a drop; otherwise,
we'd add RI triggers to the set to drop remotely, which doesn't work
because their names have OIDs embedded, and in the remote system those
are different.
One curious thing is that I had to add a hack that if an object has a
"reverse" flag in the ObjectAddresses array, also set the "normal"
output flag. (Another possibility would have been to add a "reverse"
output flag, but there doesn't seem to be a use for that --- it seems to
expose internals unnecessarily.)
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***
*** 17538,17543 FOR EACH ROW EXECUTE PROCEDURE suppress_redundant_updates_trigger();
--- 17538,17556
Object sub-id (e.g. attribute number for columns)
+ original
+ bool
+ Flag used to identify the root object of the deletion
+
+
+ normal
+ bool
+
+ Flag indicating that there's a normal dependency relationship
+ in the dependency graph leading to this object
+
+
+
object_type
text
Type of the object
***
*** 17567,17572 FOR EACH ROW EXECUTE PROCEDURE suppress_redundant_updates_trigger();
--- 17580,17601
identifier present in the identity is quoted if necessary.
+
+ address_names
+ text[]
+
+ An array that, together with address_args,
+ can be used by the C-language function getObjectAddress() to
+ recreate the object address in a remote server containing a similar object.
+
+
+
+ address_args
+ text[]
+
+ See address_names above.
+
+
*** a/src/backend/catalog/dependency.c
--- b/src/backend/catalog/dependency.c
***
*** 203,218 deleteObjectsInList(ObjectAddresses *targetObjects, Relation *depRel,
/*
* Keep track of objects for event triggers, if necessary.
*/
! if (trackDroppedObjectsNeeded())
{
for (i = 0; i < targetObjects->numrefs; i++)
{
! ObjectAddress *thisobj = targetObjects->refs + i;
!
! if ((!(flags & PERFORM_DELETION_INTERNAL)) &&
! EventTriggerSupportsObjectClass(getObjectClass(thisobj)))
{
! EventTriggerSQLDropAddObject(thisobj);
}
}
}
--- 203,227
/*
* Keep track of objects for event triggers, if necessary.
*/
! if (trackDroppedObjectsNeeded() && !(flags & PERFORM_DELETION_INTERNAL))
{
for (i = 0; i < targetObjects->numrefs; i++)
{
! const ObjectAddress *thisobj = &targetObjects->refs[i];
! const ObjectAddressExtra *extra = &targetObjects->extras[i];
! bool original = false;
! bool normal = false;
!
! if (extra->flags & DEPFLAG_ORIGINAL)
! original = true;
! if (extra->flags & DEPFLAG_NORMAL)
! normal = true;
! if (extra->flags & DEPFLAG_REVERSE)
! normal = true;
!
! if (EventTriggerSupportsObjectClass(getObjectClass(thisobj)))
{
! EventTriggerSQLDropAddObject(thisobj, original, normal);
}
}
}
*** a/src/backend/catalog/objectaddress.c
--- b/src/backend/catalog/objectaddress.c
***
*** 417,422 static const ObjectPropertyType ObjectProperty[] =
--- 417,513
}
};
+ /*
+ * This struct maps the object types as returned by getObjectTypeDescription
+ * into ObjType enum values. Note that some enum values can be obtained by
+ * different names, and that some string object types do not have corresponding
+ * values in the enum. The user of this map must be careful to test for
+ * invalid values being returned.
+ *
+ * This follows the order of getObjectTypeDescription.
+ */
+ static const struct object_type_map
+ {
+ const char *tm_name;
+ ObjectType tm_type;
+ }
+ ObjectTypeMap[] =
+ {
+ /* OCLASS_CLASS */
+ { "table", OBJECT_TABLE },
+ { "index", OBJECT_INDEX },
+ { "sequence", OBJECT_SEQUENCE },
+ { "toast table", -1 }, /* unmapped */
+ { "view", OBJECT_VIEW },
+ { "materialized view", OBJECT_MATVIEW },
+ { "composite type", OBJECT_COMPOSITE },
+ { "foreign table", OBJECT_FOREIGN_TABLE },
+ { "table column", OBJECT_COLUMN },
+ /* OCLASS_PROC */
+ { "aggregate", OBJECT_AGGREGATE },
+ { "function", OBJECT_FUNCTION },
+ /* OCLASS_TYPE */
+ { "type", OBJECT_TYPE },
+ /* OCLASS_CAST */
+ { "cast", OBJECT_CAST },
+ /* OCLASS_COLLATION */
+ { "collation", OBJECT_COLLATION },
+ /* OCLASS_CONSTRAINT */
+ { "table constraint", O
Re: [HACKERS] replicating DROP commands across servers
Tom Lane wrote:
> Alvaro Herrera writes:
> > My proposal therefore is to add some more columns to
> > pg_event_trigger_dropped_objects(): more precisely, objname and objargs,
> > which would carry exactly what get_object_address() would require to
> > re-construct an ObjectAddress for the object being dropped at the remote
> > end.
>
> Those aren't strings or indeed flat objects at all, but structures, so it
> seems like this is still rather underspecified. How will you represent
> something like a List of TypeName at the SQL level?
Yeah, that's an ugly case. I'm thinking that I could print those like
regtype output would, and then read them back in using (something
similar to) parseTypeString(). A bit convoluted perhaps, but I think it
should work. For things such as function and cast identities, typmod
shouldn't matter AFAIK, so that loss is not significant.
Another thing this will need is a table such as
static const struct
{
const char *tm_name;
ObjectType tm_type;
}
ObjectTypeMap[] =
{
/* relation types */
{ "table", OBJECT_TABLE },
{ "index", OBJECT_INDEX },
{ "sequence", OBJECT_SEQUENCE },
...
so that we can translate object types back into the ObjectType enum.
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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
Re: [HACKERS] replicating DROP commands across servers
Alvaro Herrera writes: > My proposal therefore is to add some more columns to > pg_event_trigger_dropped_objects(): more precisely, objname and objargs, > which would carry exactly what get_object_address() would require to > re-construct an ObjectAddress for the object being dropped at the remote > end. Those aren't strings or indeed flat objects at all, but structures, so it seems like this is still rather underspecified. How will you represent something like a List of TypeName at the SQL level? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
