Typo in ExecBuildSlotPartitionKeyDescription prologue

[Rushabh Lathia]

Hi

Here is a patch for fixing the function
ExecBuildSlotPartitionKeyDescription()
prologue.


Thanks,
Rushabh Lathia
www.EnterpriseDB.com
diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c
index d275cef..2fc411a 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -472,7 +472,7 @@ FormPartitionKeyDatum(PartitionDispatch pd,
 }
 
 /*
- * BuildSlotPartitionKeyDescription
+ * ExecBuildSlotPartitionKeyDescription
  *
  * This works very much like BuildIndexValueDescription() and is currently
  * used for building error messages when ExecFindPartition() fails to find

Re: [HACKERS] SQL procedures

[Craig Ringer]

On 23 November 2017 at 03:47, Andrew Dunstan  wrote:

>
>
> On 11/22/2017 02:39 PM, Corey Huinker wrote:
> >
> >
> > T-SQL procedures returns data or OUT variables.
> >
> > I remember, it was very frustrating
> >
> > Maybe first result can be reserved for OUT variables, others for
> > multi result set
> >
> >
> > It's been many years, but if I recall correctly, T-SQL returns a
> > series of result sets, with no description of the result sets to be
> > returned, each of which must be consumed fully before the client can
> > move onto the next result set. Then and only then can the output
> > parameters be read. Which was very frustrating because the OUT
> > parameters seemed like a good place to put values for things like
> > "result set 1 has 205 rows" and "X was false so we omitted one result
> > set entirely" so you couldn't, y'know easily omit entire result sets.
> >
>
>
>
> Exactly. If we want to handle OUT params this way they really need to be
> the first resultset for just this reason. That could possibly be done by
> the glue code reserving a spot in the resultset list and filling it in
> at the end of the procedure.
>

I fail to understand how that can work though. Wouldn't we have to buffer
all the resultset contents on the server in tuplestores or similar, so we
can send the parameters and then the result sets?

Isn't that going to cause a whole different set of painful difficulties
instead?

What it comes down to is that if we want to see output params before result
sets, but the output params are only emitted when the proc returns, then
someone has to buffer. We get to choose if it's the client or the server.

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

Re: [HACKERS] Parallel Append implementation

[amul sul]

On Wed, Nov 22, 2017 at 1:44 AM, Robert Haas  wrote:
> On Tue, Nov 21, 2017 at 6:57 AM, amul sul  wrote:
>> By doing following change on the v19 patch does the fix for me:
>>
>> --- a/src/backend/executor/nodeAppend.c
>> +++ b/src/backend/executor/nodeAppend.c
>> @@ -489,11 +489,9 @@ choose_next_subplan_for_worker(AppendState *node)
>> }
>>
>> /* Pick the plan we found, and advance pa_next_plan one more time. */
>> -   node->as_whichplan = pstate->pa_next_plan;
>> +   node->as_whichplan = pstate->pa_next_plan++;
>> if (pstate->pa_next_plan == node->as_nplans)
>> pstate->pa_next_plan = append->first_partial_plan;
>> -   else
>> -   pstate->pa_next_plan++;
>>
>> /* If non-partial, immediately mark as finished. */
>> if (node->as_whichplan < append->first_partial_plan)
>>
>> Attaching patch does same changes to Amit's ParallelAppend_v19_rebased.patch.
>
> Yes, that looks like a correct fix.  Thanks.
>

Attaching updated version of "ParallelAppend_v19_rebased" includes this fix.

Regards,
Amul


ParallelAppend_v20.patch
Description: Binary data

Re: has_sequence_privilege() never got the memo

[Tom Lane]

Joe Conway  writes:
> I just noticed that has_sequence_privilege() never got the memo about
> "WITH GRANT OPTION". Any objections to the attached going back to all
> supported versions?

That looks odd.  Patch certainly makes this case consistent with the
rest of acl.c, but maybe there's some deeper reason?  Peter?

regards, tom lane

Re: [HACKERS] Commits don't block for synchronous replication

[Michael Paquier]

On Thu, Nov 23, 2017 at 4:32 AM, Ashwin Agrawal  wrote:
>
> On Wed, Nov 22, 2017 at 9:57 AM, Simon Riggs  wrote:
>>
>> On 15 November 2017 at 10:07, Michael Paquier 
>> wrote:
>> > On Wed, Nov 15, 2017 at 7:28 AM, Ashwin Agrawal 
>> > wrote:
>> >>
>> >> https://commitfest.postgresql.org/15/1297/
>> >>
>> >> Am I missing something or not looking at right place, this is marked as
>> >> committed but don't see the change in latest master ?
>> >
>> > Good thing you double-checked. This has been marked as committed
>> > eleven day ago by Simon (added in CC), but no commit has happened. I
>> > am switching back the status as "ready for committer".
>>
>> The patch has been applied - look at the code. Marking back to committed.
>
> I have no idea which magical place this is being committed, atleast don't
> see on master unless checking something wrong, please can you post the
> commit here ?

I am afraid that I have to agree with Ashwin here, and would like to
know the commit number where you applied it.

The code on HEAD (and back-branches) in syncrep.c, does that, in
SyncRepWaitForLSN():
/*
 * Fast exit if user has not requested sync replication, or there are no
 * sync replication standby names defined. Note that those
standbys don't
 * need to be connected.
 */
if (!SyncRepRequested() || !SyncStandbysDefined())
return;

And the change proposed by Ashwin & co to address what is a bug is that:
/*
-* Fast exit if user has not requested sync replication, or there are no
-* sync replication standby names defined. Note that those
standbys don't
-* need to be connected.
+* Fast exit if user has not requested sync replication.
 */
-   if (!SyncRepRequested() || !SyncStandbysDefined())
+   if (!SyncRepRequested())
return;

On top of that the last commit from a certain Simon Riggs on syncrep.c
is this one:
commit: e05f6f75dbe00a7349dccf1116b5ed983b4728c0
author: Simon Riggs 
date: Fri, 12 Aug 2016 12:43:45 +0100
Code cleanup in SyncRepWaitForLSN()

This is older than the bug report of this thread. All those
indications point out that the patch has *not* been committed. So it
seems to me that you perhaps committed it to your local repository,
but forgot to push it to the remote. I am switching back the patch
status to what looks correct to me "Ready for committer". Thanks.
-- 
Michael

Re: [HACKERS] Transaction control in procedures

[Simon Riggs]

On 18 November 2017 at 02:16, Peter Eisentraut
 wrote:
> On 11/16/17 18:35, Simon Riggs wrote:
>> For the first two answers above the answer was "currently executing
>> statement", yet the third answer seems to be the procedure. So that is
>> a slight discrepancy.
>
> That's the way function execution, or really any nested execution,
> currently works.

I'm impressed that these features are so clean and simple. I wasn't
expecting that. I have very few review comments.

I vote in favour of applying these patches at the end of this CF, end of 11/17.
* Procedures
* Transaction control in PL/pgSQL (only)

That will give us 3 months to discuss problems and find solutions,
then later we can commit PL/Python, PL/perl and PL/tcl once we know
where the dragons are hiding.

If we delay, we will end up with some weird gotcha that needs changing
in the next release.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] proposal - Default namespaces for XPath expressions (PostgreSQL 11)

[Thomas Munro]

On Thu, Nov 9, 2017 at 10:11 PM, Pavel Stehule  wrote:
> Attached new version.

Hi Pavel,

FYI my patch testing robot says[1]:

 xml  ... FAILED

regression.diffs says:

+ SELECT x.* FROM t1, xmltable(XMLNAMESPACES(DEFAULT 'http://x.y'),
'/rows/row' PASSING t1.doc COLUMNS data int PATH
'child::a[1][attribute::hoge="haha"]') as x;
+ data
+ --
+ (0 rows)
+

Maybe you forgot to git-add the expected file?

[1] https://travis-ci.org/postgresql-cfbot/postgresql/builds/305979133

-- 
Thomas Munro
http://www.enterprisedb.com

Re: [PATCH] using arc4random for strong randomness matters.

[David CARLIER]

> I dunno, it seems like this is opening us to a new set of portability
> hazards (ie, sub-par implementations of arc4random) with not much gain to
> show for it.
>

Hence I reduced to three platforms only.

>
> IIUC, what this code actually does is reseed itself from /dev/urandom
> every so often and work from a PRNG in between.  That's not a layer that
> we need, because the code on top is already designed to cope with the
> foibles of /dev/urandom --- or, to the extent it isn't, that's something
> we have to fix anyway.  So it seems like having this optionally in place
> just reduces what we can assume about the randomness properties of
> pg_strong_random output, which doesn't seem like a good idea.
>
> That I admit these are valid points.
Cheers.


> regards, tom lane
>

Re: [HACKERS] SQL procedures

[Pavel Stehule]

2017-11-22 19:01 GMT+01:00 Peter Eisentraut <
peter.eisentr...@2ndquadrant.com>:

> On 11/20/17 16:25, Andrew Dunstan wrote:
> > I've been through this fairly closely, and I think it's pretty much
> > committable. The only big item that stands out for me is the issue of
> > OUT parameters.
>
> I figured that that's something that would come up.  I had intentionally
> prohibited OUT parameters for now so that we can come up with something
> for them without having to break any backward compatibility.
>
> From reading some of the references so far, I think it could be
> sufficient to return a one-row result set and have the drivers adapt the
> returned data into their interfaces.  The only thing that would be a bit
> weird about that is that a CALL command with OUT parameters would return
> a result set and a CALL command without any OUT parameters would return
> CommandComplete instead.  Maybe that's OK.
>

T-SQL procedures returns data or OUT variables.

I remember, it was very frustrating

Maybe first result can be reserved for OUT variables, others for multi
result set

Regards

Pavel



> > Default Privileges docs: although FUNCTIONS and ROUTINES are equivalent
> > here, we should probably advise using ROUTINES, as FUNCTIONS could
> > easily be take to mean "functions but not procedures".
>
> OK, I'll update the documentation a bit more.
>
> > CREATE/ALTER PROCEDURE: It seems more than a little odd to allow
> > attributes that are irrelevant to procedures in these statements. The
> > docco states "for compatibility with ALTER FUNCTION" but why do we want
> > such compatibility if it's meaningless? If we can manage it without too
> > much violence I'd prefer to see an exception raised if these are used.
>
> We can easily be more restrictive here.  I'm open to suggestions.  There
> is a difference between options that don't make sense for procedures
> (e.g., RETURNS NULL ON NULL INPUT), which are prohibited, and those that
> might make sense sometime, but are currently not used.  But maybe that's
> too confusing and we should just prohibit options that are not currently
> used.
>
> > In create_function.sgml, we have:
> >
> > If a schema name is included, then the function is created in the
> > specified schema.  Otherwise it is created in the current schema.
> > -   The name of the new function must not match any existing function
> > +   The name of the new function must not match any existing
> > function or procedure
> > with the same input argument types in the same schema.  However,
> > functions of different argument types can share a name (this is
> > called overloading).
> >
> > The last sentence should probably say "functions and procedures of
> > different argument types" There's a similar issue in
> create_procedure.sqml.
>
> fixing that
>
> > In grant.sgml, there is:
> >
> > +   The FUNCTION syntax also works for
> aggregate
> > +   functions.  Or use ROUTINE to refer to a
> > function,
> > +   aggregate function, or procedure regardless of what it is.
> >
> >
> > I would replace "Or" by "Alternatively,". I think it reads better that
> way.
>
> fixing that
>
> > In functions.c, there is:
> >
> > /* Should only get here for VOID functions */
> > -   Assert(fcache->rettype == VOIDOID);
> > +   Assert(fcache->rettype == InvalidOid || fcache->rettype
> > == VOIDOID);
> > fcinfo->isnull = true;
> > result = (Datum) 0;
> >
> > The comment should also refer to procedures.
>
> fixing that
>
> > It took me a minute to work out what is going on with the new code in
> > aclchk.c:objectsInSchemaToOids(). It probably merits a comment or two.
>
> improving that
>
> > We should document where returned values in PL procedures are ignored
> > (plperl, pltcl) and where they are not (plpython, plpgsql). Maybe we
> > should think about possibly being more consistent here, too.
>
> Yeah, suggestions?  I think it makes sense in PL/pgSQL and PL/Python to
> disallow return values that would end up being ignored, because the only
> way a return value could arise is if user code explicit calls
> RETURN/return.  However, in Perl, the return value is the result of the
> last statement, so prohibiting a return value would be very
> inconvenient.  (Don't know about Tcl.)  So maybe the current behavior
> makes sense.  Documentation is surely needed.
>
> > The context line here looks odd:
> >
> > CREATE PROCEDURE test_proc2()
> > LANGUAGE plpythonu
> > AS $$
> > return 5
> > $$;
> > CALL test_proc2();
> > ERROR:  PL/Python procedure did not return None
> > CONTEXT:  PL/Python function "test_proc2"
> >
> > Perhaps we need to change plpython_error_callback() so that "function"
> > isn't hardcoded.
>
> fixing that
>
> --
> Peter Eisentraut  http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & 

Re: [HACKERS] SQL procedures

[Tom Lane]

Peter Eisentraut  writes:
> On 11/20/17 16:25, Andrew Dunstan wrote:
>> We should document where returned values in PL procedures are ignored
>> (plperl, pltcl) and where they are not (plpython, plpgsql). Maybe we
>> should think about possibly being more consistent here, too.

> Yeah, suggestions?  I think it makes sense in PL/pgSQL and PL/Python to
> disallow return values that would end up being ignored, because the only
> way a return value could arise is if user code explicit calls
> RETURN/return.  However, in Perl, the return value is the result of the
> last statement, so prohibiting a return value would be very
> inconvenient.  (Don't know about Tcl.)  So maybe the current behavior
> makes sense.  Documentation is surely needed.

Tcl has the same approach as Perl: the return value of a proc is the
same as the value of its last command.  There's no such thing as a
proc that doesn't return a value.

regards, tom lane

Re: [HACKERS] SQL procedures

[Peter Eisentraut]

On 11/20/17 16:25, Andrew Dunstan wrote:
> I've been through this fairly closely, and I think it's pretty much
> committable. The only big item that stands out for me is the issue of
> OUT parameters.

I figured that that's something that would come up.  I had intentionally
prohibited OUT parameters for now so that we can come up with something
for them without having to break any backward compatibility.

>From reading some of the references so far, I think it could be
sufficient to return a one-row result set and have the drivers adapt the
returned data into their interfaces.  The only thing that would be a bit
weird about that is that a CALL command with OUT parameters would return
a result set and a CALL command without any OUT parameters would return
CommandComplete instead.  Maybe that's OK.

> Default Privileges docs: although FUNCTIONS and ROUTINES are equivalent
> here, we should probably advise using ROUTINES, as FUNCTIONS could
> easily be take to mean "functions but not procedures".

OK, I'll update the documentation a bit more.

> CREATE/ALTER PROCEDURE: It seems more than a little odd to allow
> attributes that are irrelevant to procedures in these statements. The
> docco states "for compatibility with ALTER FUNCTION" but why do we want
> such compatibility if it's meaningless? If we can manage it without too
> much violence I'd prefer to see an exception raised if these are used.

We can easily be more restrictive here.  I'm open to suggestions.  There
is a difference between options that don't make sense for procedures
(e.g., RETURNS NULL ON NULL INPUT), which are prohibited, and those that
might make sense sometime, but are currently not used.  But maybe that's
too confusing and we should just prohibit options that are not currently
used.

> In create_function.sgml, we have:
> 
>     If a schema name is included, then the function is created in the
>     specified schema.  Otherwise it is created in the current schema.
> -   The name of the new function must not match any existing function
> +   The name of the new function must not match any existing
> function or procedure
>     with the same input argument types in the same schema.  However,
>     functions of different argument types can share a name (this is
>     called overloading).
> 
> The last sentence should probably say "functions and procedures of
> different argument types" There's a similar issue in create_procedure.sqml.

fixing that

> In grant.sgml, there is:
> 
> +   The FUNCTION syntax also works for aggregate
> +   functions.  Or use ROUTINE to refer to a
> function,
> +   aggregate function, or procedure regardless of what it is.
> 
> 
> I would replace "Or" by "Alternatively,". I think it reads better that way.

fixing that

> In functions.c, there is:
> 
>     /* Should only get here for VOID functions */
> -   Assert(fcache->rettype == VOIDOID);
> +   Assert(fcache->rettype == InvalidOid || fcache->rettype
> == VOIDOID);
>     fcinfo->isnull = true;
>     result = (Datum) 0;
> 
> The comment should also refer to procedures.

fixing that

> It took me a minute to work out what is going on with the new code in
> aclchk.c:objectsInSchemaToOids(). It probably merits a comment or two.

improving that

> We should document where returned values in PL procedures are ignored
> (plperl, pltcl) and where they are not (plpython, plpgsql). Maybe we
> should think about possibly being more consistent here, too.

Yeah, suggestions?  I think it makes sense in PL/pgSQL and PL/Python to
disallow return values that would end up being ignored, because the only
way a return value could arise is if user code explicit calls
RETURN/return.  However, in Perl, the return value is the result of the
last statement, so prohibiting a return value would be very
inconvenient.  (Don't know about Tcl.)  So maybe the current behavior
makes sense.  Documentation is surely needed.

> The context line here looks odd:
> 
> CREATE PROCEDURE test_proc2()
> LANGUAGE plpythonu
> AS $$
> return 5
> $$;
> CALL test_proc2();
> ERROR:  PL/Python procedure did not return None
> CONTEXT:  PL/Python function "test_proc2"
> 
> Perhaps we need to change plpython_error_callback() so that "function"
> isn't hardcoded.

fixing that

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [PATCH] using arc4random for strong randomness matters.

[Andres Freund]

On November 22, 2017 8:51:07 AM PST, ilm...@ilmari.org wrote:
>If what is wanted is something more like /dev/urandom, one can call
>getentropy(2) (or on Linux, getrandom(2)) directly, which avoids having
>to open the device file each time.

What does that buy us for our usages?

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH] using arc4random for strong randomness matters.

[Dagfinn Ilmari Mannsåker]

Tom Lane  writes:

> David CARLIER  writes:
>> I m not against as such that depends of the implementation but I ve seen in
>> quick glance it s RC4 ?

arc4random uses ChaCha20 since OpenBSD 5.5 (and libbsd 0.8.0 on Linux).
It uses getentropy(2) to seed itself at regular intervals and at fork().

http://man.openbsd.org/arc4random.3

> More generally, why should we bother with an additional implementation?
> Is this better than /dev/urandom, and if so why?

If what is wanted is something more like /dev/urandom, one can call
getentropy(2) (or on Linux, getrandom(2)) directly, which avoids having
to open the device file each time.

http://man.openbsd.org/getentropy.2
https://manpages.debian.org/stretch/manpages-dev/getrandom.2.en.html

- ilmari
-- 
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
 the consequences of."  -- Skud's Meta-Law

Re: [PATCH] using arc4random for strong randomness matters.

[Andres Freund]

Hi,

Please don't top-quote on postgres mailing lists.


On 2017-11-22 16:16:35 +, David CARLIER wrote:
> > David CARLIER  writes:
> > > I m not against as such that depends of the implementation but I ve seen
> > in
> > > quick glance it s RC4 ?
> >
> > More generally, why should we bother with an additional implementation?
> > Is this better than /dev/urandom, and if so why?

> Basically the call never fails, always generating high quality random data
> (especially the implementations based on Chacha* family, RC4 has
> predictability issues), there is no need of a file descriptor.

I don't really see much benefit in those properties for postgres
specifically. Not needing an fd is nice for cases where you're not
guaranteed to have access to a filesystem, but postgres isn't going to
work in those cases anyway.

Greetings,

Andres Freund

Re: [PATCH] using arc4random for strong randomness matters.

[David CARLIER]

Basically the call never fails, always generating high quality random data
(especially the implementations based on Chacha* family, RC4 has
predictability issues), there is no need of a file descriptor.

On 22 November 2017 at 16:06, Tom Lane  wrote:

> David CARLIER  writes:
> > I m not against as such that depends of the implementation but I ve seen
> in
> > quick glance it s RC4 ?
>
> More generally, why should we bother with an additional implementation?
> Is this better than /dev/urandom, and if so why?
>
> regards, tom lane
>

Re: [PATCH] using arc4random for strong randomness matters.

[Tom Lane]

David CARLIER  writes:
> I m not against as such that depends of the implementation but I ve seen in
> quick glance it s RC4 ?

More generally, why should we bother with an additional implementation?
Is this better than /dev/urandom, and if so why?

regards, tom lane

Re: [PATCH] using arc4random for strong randomness matters.

[David CARLIER]

I m not against as such that depends of the implementation but I ve seen in
quick glance it s RC4 ?

Regards.

On 22 November 2017 at 15:37, David Fetter  wrote:

> On Tue, Nov 21, 2017 at 12:08:46PM +, David CARLIER wrote:
> > Hello,
> >
> > This is my first small personal contribution.
> >
> > Motivation :
> > - Using fail-safe, file descriptor free solution on *BSD and Darwin
> system
> > - Somehow avoiding at the moment FreeBSD as it still uses RC4 (seemingly
> > updated to Chacha20 for FreeBSD 12.0 and eventually backported later on).
> > - For implementation based on Chacha* it is known to be enough fast for
> the
> > purpose.
> > - make check passes.
> >
> > Hope it is good.
> >
> > Thanks in advance.
>
> This is neat.  Apparently, it's useable on Linux with a gizmo called
> libbsd.  Would it be worth it to test for that library on that
> platform?
>
> Best,
> David.
> --
> David Fetter  http://fetter.org/
> Phone: +1 415 235 3778
>
> Remember to vote!
> Consider donating to Postgres: http://www.postgresql.org/about/donate
>

Re: generic-msvc.h(91): error C2664

[Vicky Vergara]

Hello all


FYI: I think I solved it by not including the files and only using


extern "C" {

extern
void* SPI_palloc(size_t size);

extern void *
SPI_repalloc(void *pointer, size_t size);

extern void
SPI_pfree(void *pointer);

}


Thanks

Vicky




De: Vicky Vergara 
Enviado: martes, 21 de noviembre de 2017 09:01 p. m.
Para: pgsql-hack...@postgresql.org
Asunto: generic-msvc.h(91): error C2664


Hello all


I am trying to compile pgRouting on appveyor CI when compiling with pg >= 9.5


I am getting the following error:


C:\Program Files\PostgreSQL\9.5\include\server\port/atomics/generic-msvc.h(91): 
error C2664: '__int64 _InterlockedCompareExchange64(volatile __int64 
*,__int64,__int64)' : cannot convert argument 1 from 'volatile uint64 *' to 
'volatile __int64 *' [C:\build\pgrouting\build\pg95\x64\src\tsp\tsp.vcxproj]


This error happens when C++ code that needs palloc is being compiled.

There is no problem when compiling C code.


On the documentation of the  _InterlockedExchangeAdd64 [1] there is no uint64 * 
type

This code [2] use [3] where uint64 is used


Maybe I am doing something wrong including some postgres files on these [4] & 
[5].


A sample compilation with this error is in [6] and there is no problem at all 
with pg 9.4 [7]


Any feedback would be greatly appreciated

Vicky


[1] 
https://docs.microsoft.com/en-us/cpp/intrinsics/interlockedexchangeadd-intrinsic-functions

[2] 
https://github.com/postgres/postgres/blob/REL9_5_STABLE/src/include/port/atomics/generic-msvc.h#L101

[3] 
https://github.com/postgres/postgres/blob/REL9_5_STABLE/src/include/port/atomics/generic-msvc.h#L43

[4] 
https://github.com/cvvergara/pgrouting/blob/appveyor-pg95/include/cpp_common/pgr_alloc.hpp

[5] 
https://github.com/cvvergara/pgrouting/blob/appveyor-pg95/include/c_common/postgres_connection.h

[6] https://ci.appveyor.com/project/cvvergara/pgrouting/build/2.6.1936#L415

[7] https://ci.appveyor.com/project/cvvergara/pgrouting/build/2.6.1934

Re: [PATCH] using arc4random for strong randomness matters.

[David Fetter]

On Tue, Nov 21, 2017 at 12:08:46PM +, David CARLIER wrote:
> Hello,
> 
> This is my first small personal contribution.
> 
> Motivation :
> - Using fail-safe, file descriptor free solution on *BSD and Darwin system
> - Somehow avoiding at the moment FreeBSD as it still uses RC4 (seemingly
> updated to Chacha20 for FreeBSD 12.0 and eventually backported later on).
> - For implementation based on Chacha* it is known to be enough fast for the
> purpose.
> - make check passes.
> 
> Hope it is good.
> 
> Thanks in advance.

This is neat.  Apparently, it's useable on Linux with a gizmo called
libbsd.  Would it be worth it to test for that library on that
platform?

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Re: [HACKERS] Removing LEFT JOINs in more cases

[Ashutosh Bapat]

On Wed, Nov 1, 2017 at 5:39 AM, David Rowley
 wrote:

> In this case, the join *can* cause row duplicates, but the distinct or
> group by would filter these out again anyway, so in these cases, we'd
> not only get the benefit of not joining but also not having to remove
> the duplicate rows caused by the join.

+1.

>
> Given how simple the code is to support this, it seems to me to be
> worth handling.
>

I find this patch very simple and still useful.

@@ -597,15 +615,25 @@ rel_supports_distinctness(PlannerInfo *root,
RelOptInfo *rel)
+if (root->parse->distinctClause != NIL)
+return true;
+
+if (root->parse->groupClause != NIL && !root->parse->hasAggs)
+return true;
+

The other callers of rel_supports_distinctness() are looking for distinctness
of the given relation, whereas the code change here are applicable to any
relation, not just the given relation. I find that confusing. Looking at the
way we are calling rel_supports_distinctness() in join_is_removable() this
change looks inevitable, but still if we could reduce the confusion, that will
be good. Also if we could avoid duplication of comment about unique index, that
will be good.

DISTINCT ON allows only a subset of columns being selected to be listed in that
clause. I initially thought that specifying only a subset would be a problem
and we should check whether the DISTINCT applies to all columns being selected.
But that's not true, since DISTINCT ON would eliminate any duplicates in the
columns listed in that clause, effectively deduplicating the row being
selected. So, we are good there. May be you want to add a testcase with
DISTINCT ON.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Logical Replication and PgPool

[Taylor, Nathaniel N.]

All,
I have high availability multi instance implementation of PostgreSQL using 
PgPooL v3.6. I am thinking of doing data separation and change data capture 
using logical replication . Any ideas?


Thank you
Dr. Nathaniel Taylor, Ph.D (Comp. Sci.),C|EH,CASP
LM, CPO, DCPS2- Enterprise  Systems  Database  Architect
Cell : (202) 415-1342

-Original Message-
From: Robert Haas [mailto:robertmh...@gmail.com] 
Sent: Tuesday, November 21, 2017 4:35 PM
To: Simon Riggs 
Cc: Thomas Rosenstein ; Craig Ringer 
; PostgreSQL Hackers 
Subject: Re: Logical Replication and triggers

On Tue, Nov 21, 2017 at 4:28 PM, Simon Riggs  wrote:
> I would have acted on this myself a few days back if I thought the 
> patch was correct, but I see multiple command counter increments 
> there, so suspect an alternate fix is correct.

Oh, well, I'm glad you said something.  I was actually thinking about 
committing the patch Peter posted in 
http://postgr.es/m/619c557d-93e6-1833-1692-b010b176f...@2ndquadrant.com
because it looks correct to me, but I'm certainly not an expert on that code so 
I'll wait for your analysis.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company

Re: [HACKERS] Parallel Hash take II

[Thomas Munro]

Hi Andres and Peter,

Here's a new patch set with responses to the last batch of review comments.

On Wed, Nov 15, 2017 at 10:24 AM, Andres Freund  wrote:
> Hm. The way you access this doesn't quite seem right:
> ...
> +  matches := regexp_matches(line, '  Batches: ([0-9]+)');
> ...
>
> Why not use format json and access the output that way? Then you can be
> sure you access the right part of the tree and such?

Okay, done.

>> 1.  It determines the amount of unfairness when we run out of data:
>> it's the maximum amount of extra data that the unlucky last worker can
>> finish up with after all the others have finished.  I think this
>> effect is reduced by higher level factors: when a reader runs out of
>> data in one backend's file, it'll start reading another backend's
>> file.  If it's hit the end of all backends' files and this is an outer
>> batch, Parallel Hash will just go and work on another batch
>> immediately.
>
> Consider e.g. what happens if there's the occasional 500MB datum, and
> the rest's very small...
>
>> Better ideas?
>
> Not really. I'm more than a bit suspicous of this solution, but I don't
> really have a great suggestion otherwise.  One way to combat extreme
> size skew would be to put very large datums into different files.

Right.  I considered opening a separate file for each chunk size (4
page, 8 page, 16 page, ...).  Then each file has uniform chunk size,
so you're not stuck with a large chunk size after one monster tuple
comes down the pipe.  I didn't propose that because it leads to even
more file descriptors being used.  I'd like to move towards fewer file
descriptors, because it's a cap on the number of partitions you can
reasonably use.  Perhaps in future we could develop some kind of
general purpose file space manager that would let us allocate extents
within a file, and then SharedTuplestore could allocate extents for
each chunk size.  Not today though.

> But I think we probably can go with your approach for now, ignoring my
> failure prone spidey senses ;)

Cool.

>> > This looks more like the job of an lwlock rather than a spinlock.
>>
>> ... assembler ...
>>
>> That should be OK, right?
>
> It's not too bad. Personally I'm of the opinion though that pretty much
> no new spinlocks should be added - their worst case performance
> characteristics are bad enough for that to be only worth the
> experimentation in case swhere each cycle really matters and where
> contention is unlikely.

Changed to LWLock.

>> > One day we're going to need a better approach to this. I have no idea
>> > how, but this per-node, and now per_node * max_parallelism, approach has
>> > only implementation simplicity as its benefit.
>>
>> I agree, and I am interested in that subject.  In the meantime, I
>> think it'd be pretty unfair if parallel-oblivious hash join and
>> sort-merge join and every other parallel plan get to use work_mem * p
>> (and in some cases waste it with duplicate data), but Parallel Hash
>> isn't allowed to do the same (and put it to good use).
>
> I'm not sure I care about fairness between pieces of code ;)

I'll leave that discussion to run in a separate subthread...

>> BTW this is not per-tuple code -- it runs once at the end of hashing.
>> Not sure what you're looking for here.
>
> It was more a general statement about all the branches in nodeHashjoin,
> than about these specific branches. Should've made that clearer. There's
> definitely branches in very common parts:
>
> ...
>
> I don't think you should do so now, but I think a reasonable approach
> here would be to move the HJ_BUILD_HASHTABLE code into a separate
> function (it really can't be hot). Then have specialized ExecHashJoin()
> versions for parallel/non-parallel and potentially for outer/inner/anti.

Okay, here's my proposal for how to get new branches out of the
per-tuple path.  I have separated ExecHashJoin() and
ExecParallelHashJoin() functions.  They call an inline function
ExecHashJoinImpl() with a constant parameter, so that I effectively
instantiate two variants with the unwanted branches removed by
constant folding.  Then the appropriate variant is installed as the
ExecProcNode function pointer.

Just "inline" wasn't enough though.  I defined
pg_attribute_always_inline to force that on GCC/Clang et al and MSVC.

I also created a separate function ExecParallelScanHashBucket().  I
guess I could have extended the above trick into ExecScanHashBucket()
and further too, but it's called from a different translation unit,
and I also don't want to get too carried away with this technique.  I
chose to supply different functions.

So -- that's introducing a couple of new techniques into the tree.
Treating ExecProcNode as a configuration point for a single node type,
and the function instantiation trick.  Thoughts?

>> > If we don't split this into two versions, we at least should store
>> > hashNode->parallel_state in a local var, so the compiler doesn't have to
>> > pull that out of 

Re: [HACKERS] UPDATE of partition key

[Amit Khandekar]

On 21 November 2017 at 17:24, Amit Khandekar  wrote:
> On 13 November 2017 at 18:25, David Rowley  
> wrote:
>>
>> 30. The following chunk of code is giving me a headache trying to
>> verify which arrays are which size:
>>
>> ExecSetupPartitionTupleRouting(rel,
>>mtstate->resultRelInfo,
>>(operation == CMD_UPDATE ? nplans : 0),
>>node->nominalRelation,
>>estate,
>>_dispatch_info,
>>,
>>_tupconv_maps,
>>_leaf_map,
>>_tuple_slot,
>>_parted, _partitions);
>> mtstate->mt_partition_dispatch_info = partition_dispatch_info;
>> mtstate->mt_num_dispatch = num_parted;
>> mtstate->mt_partitions = partitions;
>> mtstate->mt_num_partitions = num_partitions;
>> mtstate->mt_parentchild_tupconv_maps = partition_tupconv_maps;
>> mtstate->mt_subplan_partition_offsets = subplan_leaf_map;
>> mtstate->mt_partition_tuple_slot = partition_tuple_slot;
>> mtstate->mt_root_tuple_slot = MakeTupleTableSlot();
>>
>> I know this patch is not completely responsible for it, but you're not
>> making things any better.
>>
>> Would it not be better to invent some PartitionTupleRouting struct and
>> make that struct a member of ModifyTableState and CopyState, then just
>> pass the pointer to that struct to ExecSetupPartitionTupleRouting()
>> and have it fill in the required details? I think the complexity of
>> this is already on the high end, I think you really need to do the
>> refactor before this gets any worse.
>>
>
>Ok. I am currently working on doing this change. So not yet included in the 
>attached patch. Will send yet another revised patch for this change.

Attached incremental patch encapsulate_partinfo.patch (to be applied
over the latest v25 patch) contains changes to move all the
partition-related information into new structure
PartitionTupleRouting. Further to that, I also moved
PartitionDispatchInfo into this structure. So it looks like this :

typedef struct PartitionTupleRouting
{
PartitionDispatch *partition_dispatch_info;
int num_dispatch;
ResultRelInfo **partitions;
int num_partitions;
TupleConversionMap **parentchild_tupconv_maps;
int*subplan_partition_offsets;
TupleTableSlot *partition_tuple_slot;
TupleTableSlot *root_tuple_slot;
} PartitionTupleRouting;

So this structure now encapsulates *all* the
partition-tuple-routing-related information. So ModifyTableState now
has only one tuple-routing-related field 'mt_partition_tuple_routing'.
It is changed like this :

@@ -976,24 +976,14 @@ typedef struct ModifyTableState
TupleTableSlot *mt_existing;/* slot to store existing
target tuple in */
List   *mt_excludedtlist;   /* the excluded pseudo
relation's tlist  */
TupleTableSlot *mt_conflproj;   /* CONFLICT ... SET ...
projection target */
-   struct PartitionDispatchData **mt_partition_dispatch_info;
-   /* Tuple-routing support info */
-   int mt_num_dispatch;/* Number of
entries in the above array */
-   int mt_num_partitions;  /* Number of
members in the following
-
  * arrays */
-   ResultRelInfo **mt_partitions;  /* Per partition result
relation pointers */
-   TupleTableSlot *mt_partition_tuple_slot;
-   TupleTableSlot *mt_root_tuple_slot;
+   struct PartitionTupleRouting *mt_partition_tuple_routing; /*
Tuple-routing support info */
struct TransitionCaptureState *mt_transition_capture;
/* controls transition table population for specified operation */
struct TransitionCaptureState *mt_oc_transition_capture;
/* controls transition table population for INSERT...ON
CONFLICT UPDATE */
-   TupleConversionMap **mt_parentchild_tupconv_maps;
-   /* Per partition map for tuple conversion from root to leaf */
TupleConversionMap **mt_childparent_tupconv_maps;
/* Per plan/partition map for tuple conversion from child to root */
boolmt_is_tupconv_perpart;  /* Is the above map
per-partition ? */
-   int *mt_subplan_partition_offsets;
/* Stores position of update result rels in leaf partitions */
 } ModifyTableState;

So the code in nodeModifyTable.c (and similar code in copy.c) is
accordingly adjusted to use mtstate->mt_partition_tuple_routing.

The places where we use (mtstate->mt_partition_dispatch_info != NULL)
condition to run tuple-routing code, I have replaced it with
mtstate->mt_partition_tuple_routing != NULL.

If you are ok with the incremental patch, I can extract this change
into a separate preparatory patch to be applied on PG master.

Thanks
-Amit Khandekar


encapsulate_partinfo.patch
Description: Binary data

Re: [HACKERS] [PATCH] Incremental sort

[Antonin Houska]

Alexander Korotkov  wrote:

> Antonin Houska  wrote:

> >  * ExecIncrementalSort()
> >
> >  ** if (node->tuplesortstate == NULL)
> > 
> >  If both branches contain the expression
> > 
> >  node->groupsCount++;
> > 
> >  I suggest it to be moved outside the "if" construct.
> 
> Done.

One more comment on this: I wonder if the field isn't incremented too
early. It seems to me that the value can end up non-zero if the input set is
to be empty (not sure if it can happen in practice).

And finally one question about regression tests: what's the purpose of the
changes in contrib/postgres_fdw/sql/postgres_fdw.sql ? I see no
IncrementalSort node in the output.

-- 
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de, http://www.cybertec.at

Re: [HACKERS] UPDATE of partition key

[Amit Langote]

Thanks Amit.

Looking at the latest v25 patch.

On 2017/11/16 23:50, Amit Khandekar wrote:
> Below has the responses for both Amit's and David's comments, starting
> with Amit's 
> On 2 November 2017 at 12:40, Amit Langote  
> wrote:
>> On 2017/10/24 0:15, Amit Khandekar wrote:
>>> On 16 October 2017 at 08:28, Amit Langote  
>>> wrote:

 +   (event == TRIGGER_EVENT_UPDATE && ((oldtup == NULL) ^ (newtup
 == NULL

 Is there some reason why a bitwise operator is used here?
>>>
>>> That exact condition means that the function is called for transition
>>> capture for updated rows being moved to another partition. For this
>>> scenario, either the oldtup or the newtup is NULL. I wanted to exactly
>>> capture that condition there. I think the bitwise operator is more
>>> user-friendly in emphasizing the point that it is indeed an "either a
>>> or b, not both" condition.
>>
>> I see.  In that case, since this patch adds the new condition, a note
>> about it in the comment just above would be good, because the situation
>> you describe here seems to arise only during update-tuple-routing, IIUC.
> 
> Done. Please check.

Looks fine.

>> + * 'update_rri' has the UPDATE per-subplan result rels. These are re-used
>> + *  instead of allocating new ones while generating the array of all 
>> leaf
>> + *  partition result rels.
>>
>> Instead of:
>>
>> "These are re-used instead of allocating new ones while generating the
>> array of all leaf partition result rels."
>>
>> how about:
>>
>> "There is no need to allocate a new ResultRellInfo entry for leaf
>> partitions for which one already exists in this array"
> 
> Ok. I have made it like this :
> 
> + * 'update_rri' contains the UPDATE per-subplan result rels. For the
> output param
> + * 'partitions', we don't allocate new ResultRelInfo objects for
> + * leaf partitions for which they are already available
> in 'update_rri'.

Sure.

>>> It looks like the interface does not much simplify, and above that, we
>>> have more number of lines in that function. Also, the caller anyway
>>> has to be aware whether the map_index is the index into the leaf
>>> partitions or the update subplans. So it is not like the caller does
>>> not have to be aware about whether the mapping should be
>>> mt_persubplan_childparent_maps or mt_perleaf_parentchild_maps.
>>
>> Hmm, I think we should try to make it so that the caller doesn't have to
>> be aware of that.  And by caller I guess you mean ExecInsert(), which
>> should not be a place, IMHO, where to try to introduce a lot of new logic
>> specific to update tuple routing.
> 
> I think, for ExecInsert() since we have already given the job of
> routing the tuple from root partitioned table to a partition, it makes
> sense to give the function the additional job of routing the tuple
> from any partition to any partition. ExecInsert() can be looked at as
> doing this job : "insert a tuple into the right partition; the
> original tuple can belong to any partition"

Yeah, that's one way of looking at that.  But I think ExecInsert() as it
is today thinks it's got a *new* tuple and that's it.  I think the newly
introduced code in it to find out that it is not so (that the tuple
actually comes from some other partition), that it's really the
update-turned-into-delete-plus-insert, and then switch to the root
partitioned table's ResultRelInfo, etc. really belongs outside of it.
Maybe in its caller, which is ExecUpdate().  I mean why not add this code
to the block in ExecUpdate() that handles update-row-movement.

Just before calling ExecInsert() to do the re-routing seems like a good
place to do all that.  For example, try the attached incremental patch
that applies on top of yours.  I can see after applying it that diffs to
ExecInsert() are now just some refactoring ones and there are no
significant additions making it look like supporting update-row-movement
required substantial changes to how ExecInsert() itself works.

> After doing the changes for the int[] array map in the previous patch
> version, I still feel that ConvertPartitionTupleSlot() should be
> retained. We save some repeated lines of code saved.

OK.

>> You may be right, but I see for WithCheckOptions initialization
>> specifically that the non-tuple-routing code passes the actual sub-plan
>> when initializing the WCO for a given result rel.
> 
> Yes that's true. The problem with WithCheckOptions for newly allocated
> partition result rels is : we can't use a subplan for the parent
> parameter because there is no subplan for it. But I will still think
> on it a bit more (TODO).

Alright.

>>> I think you are suggesting we do it like how it's done in
>>> is_partition_attr(). Can you please let me know other places we do
>>> this same way ? I couldn't find.
>>
>> OK, not as many as I thought there would be, but there are following
>> beside 

Re: With commit 4e5fe9ad19, range partition missing handling for the NULL partition key

[amul sul]

On Wed, Nov 22, 2017 at 11:38 AM, Amit Langote
 wrote:
> Hi Rushabh,
>
> On 2017/11/22 13:45, Rushabh Lathia wrote:
>> Consider the below test:
>>
>> CREATE TABLE range_tab(a int, b int) PARTITION BY RANGE(a);
>> CREATE TABLE range_tab_p1 PARTITION OF range_tab FOR VALUES FROM (minvalue)
>> TO (10);
>> CREATE TABLE range_tab_p2 PARTITION OF range_tab FOR VALUES FROM (10) TO
>> (20);
>> CREATE TABLE range_tab_p3 PARTITION OF range_tab FOR VALUES FROM (20) TO
>> (maxvalue);
>>
>> INSERT INTO range_tab VALUES(NULL, 10);
>>
>> Above insert should fail with an error "no partition of relation found for
>> row".
>>
>> Looking further I found that, this behaviour is changed after below commit:
>>
>> commit 4e5fe9ad19e14af360de7970caa8b150436c9dec
>> Author: Robert Haas 
>> Date:   Wed Nov 15 10:23:28 2017 -0500
>>
>> Centralize executor-related partitioning code.
>>
>> Some code is moved from partition.c, which has grown very quickly
>> lately;
>> splitting the executor parts out might help to keep it from getting
>> totally out of control.  Other code is moved from execMain.c.  All is
>> moved to a new file execPartition.c.  get_partition_for_tuple now has
>> a new interface that more clearly separates executor concerns from
>> generic concerns.
>>
>> Amit Langote.  A slight comment tweak by me.
>>
>> Before above commit insert with NULL partition key in the range partition
>> was throwing a proper error.
>
> Oops, good catch.
>
>> Attaching patch to fix as well as regression test.
>
> Thanks for the patch.  About the code, how about do it like the attached
> instead?
>

Looks good to me, even we can skip the following change in v2 patch:

19 @@ -2560,6 +2559,8 @@ get_partition_for_tuple(Relation relation,
Datum *values, bool *isnull)
 20  */
 21 part_index =
partdesc->boundinfo->indexes[bound_offset + 1];
 22 }
 23 +   else
 24 +   part_index = partdesc->boundinfo->default_index;
 25 }
 26 break;
 27

default_index will get assign by following code in get_partition_for_tuple() :

   /*
 * part_index < 0 means we failed to find a partition of this parent.
 * Use the default partition, if there is one.
 */
if (part_index < 0)
part_index = partdesc->boundinfo->default_index;


Regards,
Amul