replicating DROP commands across servers

Alvaro Herrera <alvherre@2ndquadrant.com> 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

Tom Lane wrote:

Alvaro Herrera <alvherre@2ndquadrant.com> 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 Herrera 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

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 Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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

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

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 Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com

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

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 Herrera 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

Alvaro,

* Alvaro Herrera (alvherre@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

On 10/03/2014 08:08 PM, Stephen Frost wrote:

Alvaro,

* Alvaro Herrera (alvherre@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:

+        <row>
+         <entry><literal>address_names</literal></entry>
+         <entry><type>text[]</type></entry>
+         <entry>
+          An array that, together with <literal>address_args</literal>,
+          can be used by the C-language function getObjectAddress() to
+          recreate the object address in a remote server containing a similar object.
+         </entry>
+        </row>
+        <row>
+         <entry><literal>address_args</literal></entry>
+         <entry><type>text[]</type></entry>
+         <entry>
+          See <literal>address_names</literal> above.
+         </entry>
+        </row>

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

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:

+        <row>
+         <entry><literal>address_names</literal></entry>
+         <entry><type>text[]</type></entry>
+         <entry>
+          An array that, together with <literal>address_args</literal>,
+          can be used by the C-language function getObjectAddress() to
+          recreate the object address in a remote server containing a similar object.
+         </entry>
+        </row>
+        <row>
+         <entry><literal>address_args</literal></entry>
+         <entry><type>text[]</type></entry>
+         <entry>
+          See <literal>address_names</literal> above.
+         </entry>
+        </row>

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 Herrera 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

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:

+        <row>
+         <entry><literal>address_names</literal></entry>
+         <entry><type>text[]</type></entry>
+         <entry>
+          An array that, together with <literal>address_args</literal>,
+          can be used by the C-language function getObjectAddress() to
+          recreate the object address in a remote server containing a similar object.
+         </entry>
+        </row>
+        <row>
+         <entry><literal>address_args</literal></entry>
+         <entry><type>text[]</type></entry>
+         <entry>
+          See <literal>address_names</literal> above.
+         </entry>
+        </row>

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

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 Herrera 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

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 <obj> 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 Herrera 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

Alvaro,

* Alvaro Herrera (alvherre@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

Stephen Frost wrote:

Alvaro,

* Alvaro Herrera (alvherre@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 Herrera 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

* Alvaro Herrera (alvherre@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

Stephen Frost wrote:

* Alvaro Herrera (alvherre@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 Herrera 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