Re: Logical Replication WIP

On 04/11/16 14:24, Andres Freund wrote:
> Hi,
> 
> (btw, I vote against tarballing patches)
>

Well, I vote against CF app not handling correctly emails with multiple
attachments :)


> +   
> +    
> +     
> +      Name
> +      Type
> +      References
> +      Description
> +     
> +    
> +
> +    
> +     
> +      oid
> +      oid
> +      
> +      Row identifier (hidden attribute; must be explicitly selected)
> +     
> +
> 
> +     
> +      subpublications
> +      name[]
> +      
> +      Array of subscribed publication names. These reference the
> +       publications on the publisher server.
> +      
> 
> Why is this names and not oids? So you can see it across databases?
> 

Because they only exist on remote server.

> 
> 
>  include $(top_srcdir)/src/backend/common.mk
> diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
> index 68d7e46..523008d 100644
> --- a/src/backend/commands/event_trigger.c
> +++ b/src/backend/commands/event_trigger.c
> @@ -112,6 +112,7 @@ static event_trigger_support_data event_trigger_support[] = {
>  	{"SCHEMA", true},
>  	{"SEQUENCE", true},
>  	{"SERVER", true},
> +	{"SUBSCRIPTION", true},
> 
> Hm, is that ok? Subscriptions are shared, so ...?
> 

Good point, forgot event triggers don't handle shared objects.

> 
> +		/*
> +		 * If requested, create the replication slot on remote side for our
> +		 * newly created subscription.
> +		 *
> +		 * Note, we can't cleanup slot in case of failure as reason for
> +		 * failure might be already existing slot of the same name and we
> +		 * don't want to drop somebody else's slot by mistake.
> +		 */
> +		if (create_slot)
> +		{
> +			XLogRecPtr			lsn;
> +
> +			/*
> +			 * Create the replication slot on remote side for our newly created
> +			 * subscription.
> +			 *
> +			 * Note, we can't cleanup slot in case of failure as reason for
> +			 * failure might be already existing slot of the same name and we
> +			 * don't want to drop somebody else's slot by mistake.
> +			 */
> 
> We should really be able to recognize that based on the error code...
> 

We could, provided that the slot is active, but that would leave nasty
race condition where if you do drop and the other subscription of same
name is not running (restarting, temporarily disabled, etc) we'll remove
the slot for it. Maybe we should not care about that and say slot is
representing the subscription and if you name slot same for two
different subscriptions then that's your problem though.

> +/*
> + * Drop subscription by OID
> + */
> +void
> +DropSubscriptionById(Oid subid)
> +{
> 
> +	/*
> +	 * We must ignore errors here as that would make it impossible to drop
> +	 * subscription when publisher is down.
> +	 */
> 
> I'm not convinced.  Leaving a slot around without a "record" of it on
> the creating side isn't nice either. Maybe a FORCE flag or something?
> 

I would like to have this as option yes, not sure if FORCE is best
naming, but I have trouble coming up with good name. We have CREATE_SLOT
and NOCREATE_SLOT for CREATE SUBSCRIPTION, so maybe we could have
DROP_SLOT (default) and NODROP_SLOT for DROP SUBSCRIPTION.


--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services