Re: replication slot restart_lsn initialization

From: Gurjeet Singh <gurjeet(at)singh(dot)im>
To: Andres Freund <andres(at)anarazel(dot)de>
Cc: "Duran, Danilo" <danilod(at)amazon(dot)com>, "robertmhaas(at)gmail(dot)com" <robertmhaas(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: replication slot restart_lsn initialization
Date: 2015-07-07 16:42:54
Message-ID: CABwTF4Wh_dBCzTU=49pFXR6coR4NW1ynb+vBqT+Po=7fuq5iCw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

On Tue, Jul 7, 2015 at 6:49 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:

> On 2015-07-07 06:41:55 -0700, Gurjeet Singh wrote:
> > There seems to be a misplaced not operator ! in that if statement, as
> > well. That sucks :( The MacOS gcc binary is actually clang, and its
> output
> > is too noisy [1], which is probably why I might have missed warning if
> any.
>
> Which version of clang is it? With newer ones you can individually
> disable nearly all of the warnings. I regularly use clang, and most of
> the time it compiles master without warnings.
>

> $ gcc --version
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr
--with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 6.1.0 (clang-602.0.53) (based on LLVM 3.6.0svn)
Target: x86_64-apple-darwin14.4.0
Thread model: posix

I see that -Wno-parentheses-equality might help, but I am not looking
forward to maintaining OS specific flags for clang-that-pretends-to-be-gcc
in my shell scripts [2]

[2] https://github.com/gurjeet/pgd

Attached is a patch that introduces SlotIsPhyscial/SlotIsLogical macros.
This patch, if accepted, goes on top of the v4 patch.

> > > > pg_create_physical_replication_slot(PG_FUNCTION_ARGS)
> > > > {
> > > > Name name = PG_GETARG_NAME(0);
> > > > + bool activate = PG_GETARG_BOOL(1);
> > >
> > > Don't like 'activate' much as a name. How about 'immediately_reserve'?
> > >
> >
> > I still like 'activate, but okay. How about 'reserve_immediately'
> instead?
>
> Activate is just utterly wrong. A slot isn't inactive before. And
> 'active' already is used for slots that are currently in use
> (i.e. connected to).
>
> > Also, do you want this name change just in the C code, or in the pg_proc
> > and docs as well?
>
> All.

Patch v4 attached.

On a side note, I see that the pg_create_*_replication_slot() functions do
not behave transactionally; that is, rolling back a transaction does not
undo the slot creation. Should we prevent these, and corresponding drop
functions, from being called inside an explicit transaction?
PreventTransactionChain() is geared towards serving just the utility
commands. Do we protect against calling such functions in a transaction
block, or from user functions? How?

Best regards,
--
Gurjeet Singh http://gurjeet.singh.im/

Attachment Content-Type Size
physical_repl_slot_activate_restart_lsn.v4.patch application/octet-stream 10.6 KB
slot_is_physical_or_logical_macros.patch application/octet-stream 3.6 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Bruce Momjian 2015-07-07 16:44:29 Re: Missing latex-longtable value
Previous Message Heikki Linnakangas 2015-07-07 16:32:54 Re: Improving test coverage of extensions with pg_dump