Re: walsender & parallelism

On 25/04/17 01:25, Andres Freund wrote:
> Hi,
>
> On 2017-04-24 07:31:18 +0200, Petr Jelinek wrote:
>> The previous coding tried to run the unknown string throur lexer which
>> could fail for some valid SQL statements as the replication command
>> parser is too simple to handle all the complexities of SQL language.
>>
>> Instead just fall back to SQL parser on any unknown command.
>>
>> Also remove SHOW command handling from walsender as it's handled by the
>> simple query code path.
>
> This break SHOW var; over the plain replication connections though
> (which was quite intentionally supported), so I don't think that's ok?
>

Hmm I guess we don't use this anywhere in tests (which is not surprising
as it would have to be used in plain walsender connection). Fixed.

>
> I don't like much how SHOW and walsender understanding SQL statements
> turned out, I think code like
>
> if (am_walsender)
> {
> if (!exec_replication_command(query_string))
> exec_simple_query(query_string);
> }
> else
> exec_simple_query(query_string);
>
> shows some of the issues here.
>

Not sure how it's related to SHOW, but in any case, it's just result of
the parsing fallback.

>
>> New alternative output for largeobject test has been added as the
>> replication connection does not support fastpath function calls.
>
> I think just supporting fastpath over the replication protocol is less
> work than maintaining the alternative file.
>
>
>> --- a/src/Makefile.global.in
>> +++ b/src/Makefile.global.in
>> @@ -321,6 +321,7 @@ BZIP2 = bzip2
>> # Testing
>>
>> check: temp-install
>> +check-walsender-sql: temp-install
>
> This doesn't strike me as something that should go into
> a/src/Makefile.global.in - I'd rather put it in
> b/src/test/regress/GNUmakefile. check is in .global because it's used
> in a lot of different makefiles, but that's not true for this target, right?
>

Shows how well I know our build system :)

Anyway, I gave it a try to support as much as possible and the result is
attached.

First patch adds the new make target that runs regression tests through
walsender. I had to change one test to run SHOW TIMEZONE instead of SHOW
TIME ZONE, otherwise no tests need to be changed.

The second patch unifies the sighup handling across all kinds of
processes. This is mostly replacing got_SIGHuP with new variable and
removing local SIGHUP handlers in favor of generic one where possible.

And the third patch changes walsender. There are 3 things it does a) it
overhauls signal handling there, b) it allows both fast path function
calls and extended protocol messages (once I did function fast path I
didn't really see any difference in terms of extended protocol, please
correct me if I am wrong in that) and c) improves the fallback in parser.

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