date:20171115

Hi

postgres=# select i, i from generate_series(1,5) g(i);
┌─[ RECORD 1 ]─┐
│ i │ 1 │
│ i │ 1 │
╞═[ RECORD 2 ]═╡
│ i │ 2 │
│ i │ 2 │
╞═[ RECORD 3 ]═╡
│ i │ 3 │
│ i │ 3 │
╞═[ RECORD 4 ]═╡
│ i │ 4 │
│ i │ 4 │
╞═[ RECORD 5 ]═╡
│ i │ 5 │
│ i │ 5 │
└───┴───┘

The last column need to be more wider

Regards

Pavel

Re: Further simplification of c.h's #include section

On Thu, Nov 16, 2017 at 2:23 PM, David Rowley
 wrote:
> I do get some warnings:
>
> (ClCompile target) ->
>   src/test/modules/test_session_hooks/test_session_hooks.c(112): warning
> C4113: 'void (__cdecl *)()' differs in parameter lists from
> 'session_start_hook_type' [test_session_hooks.vcxproj]
>   src/test/modules/test_session_hooks/test_session_hooks.c(113): warning
> C4113: 'void (__cdecl *)()' differs in parameter lists from
> 'session_end_hook_type' [test_session_hooks.vcxproj].
>
> 2 Warning(s)
>
> But I imagine that was caused by 745948422c (also get the warnings without
> your patch)

Do the warnings go away with the patch attached?
-- 
Michael


session_hook_void.patch
Description: Binary data

Re: [HACKERS] Assertion failure when the non-exclusive pg_stop_backup aborted.

On Thu, Nov 16, 2017 at 10:57 AM, Masahiko Sawada  wrote:
> Agreed. Attached the updated patch, please review it.

+   /*
+* Quick exit if session is not keeping around a non-exclusive backup
+* already started.
+*/
+   if (sessionBackupState != SESSION_BACKUP_NON_EXCLUSIVE)
+   return;
I think that it would be more solid to use SESSION_BACKUP_NONE for the
comparison, and complete the assertion after the quick exit as follows
as this code path should never be taken for an exclusive backup:
+   Assert(XLogCtl->Insert.nonExclusiveBackups > 0 &&
+  sessionBackupState == SESSION_BACKUP_NON_EXCLUSIVE);

And your patch would discard both SESSION_BACKUP_EXCLUSIVE and
SESSION_BACKUP_NONE.
-- 
Michael

Re: [HACKERS] path toward faster partition pruning

2017-11-15 Thread David Rowley

On 16 November 2017 at 15:54, Kyotaro HORIGUCHI
 wrote:
> Anyway I added __attribute((noinline)) to the two
> functions and got the following result.
>
>> bms_add_range in 1.24 (12.4 ns per loop)
>> bms_add_range2 in 0.8 (8 ns per loop)

I see similar here with __attribute((noinline)). Thanks for
investigating that. Your way is clearly better. Thanks for suggesting
it.

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

Re: [HACKERS] Timeline ID in backup_label file

On Thu, Nov 16, 2017 at 9:20 AM, David Steele  wrote:
> For this patch at least, I think we should do #1.  Getting rid of the order
> dependency is attractive but there may be other programs that are depending
> on the order.  I know you are not proposing to change the order now, but it
> *could* be changed with #2.

read_backup_label() is a static function in the backend code. With #2
I do not imply to change the order of the elements written in the
backup_label file, just to make the way they are parsed smarter.

> Also, I think DEBUG1 would be a more appropriate log level if any logging is
> done.

OK, the label and time strings can include spaces. The label string is
assumed to not be bigger than 1028 bytes, and the timestamp is assumed
that it can get up to 128. So it is possible to use stuff like
fscanf("%127[^\n]\n") to avoid any buffer overflow problems on the
backend. If a backup_label is generated with strings longer than that,
then read_backup_label would fail to parse the next entries but that's
not really something to worry about IMO as those are only minor sanity
checks. Having a safer coding in the backend is more important, and
the new checks should not trigger any hard failures.
-- 
Michael


backup_label_tli_v2.patch
Description: Binary data

Re: [HACKERS] ginInsertCleanup called from vacuum could still miss tuples to be deleted

On Mon, Nov 13, 2017 at 3:25 AM, Masahiko Sawada  wrote:
> In ginInsertCleanup(), we lock the GIN meta page by LockPage and could
> wait for the concurrent cleaning up process if stats == NULL. And the
> source code comment says that this happen is when ginINsertCleanup is
> called by [auto]vacuum/analyze or gin_clean_pending_list(). I agree
> with this behavior. However, looking at the callers the stats is NULL
> only either if pending list exceeds to threshold during insertions or
> if only analyzing is performed by an autovacum worker or ANALYZE
> command. So I think we should inVacuum = (stats != NULL) instead.

Argh.  Yeah, that looks wrong.

Instead of relying on this indirect method, how about passing an
explicit inVacuum argument to that function?  And while we're at it,
how about renaming inVacuum to forceCleanup?

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

Re: [HACKERS] moving some partitioning code to executor

2017-11-15 Thread Amit Langote

On 2017/11/16 0:29, Robert Haas wrote:
> On Tue, Nov 14, 2017 at 7:59 PM, Amit Langote
>  wrote:
>> On 2017/11/15 2:09, Alvaro Herrera wrote:
>>> Amit Langote wrote:
>>>
 Since that 0001 patch was committed [1], here is the rebased patch.  Will
 add this to the November commit-fest.
>>>
>>> Please rebase once more -- 1aba8e651ac3 seems to have broken things
>>> in this area pretty thoroughly.
>>
>> Thanks, done.
> 
> Committed.

Thank you.

Regards,
Amit

Re: Further simplification of c.h's #include section

On Wed, Nov 15, 2017 at 5:22 PM, Tom Lane  wrote:
> Thoughts?

Sure, having a win32_port.h as a sub-include of port.h seems fine.

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

Re: [HACKERS] Assertion failure when the non-exclusive pg_stop_backup aborted.

2017-11-15 Thread Masahiko Sawada

On Wed, Nov 15, 2017 at 5:39 PM, Michael Paquier
 wrote:
> On Wed, Nov 15, 2017 at 5:21 PM, Masahiko Sawada  
> wrote:
>> On Wed, Nov 15, 2017 at 2:38 PM, Michael Paquier
>>  wrote:
>>> On Wed, Nov 15, 2017 at 12:12 PM, Masahiko Sawada  
>>> wrote:
 I think we need to check only sessionBackupState and don't need to
 check XLogCtl->Insert.exclusiveBackupState in do_pg_abort_backup(). We
 can quickly return if sessionBackupState !=
 SESSION_BACKUP_NON_EXCLUSIVE. In your suggestion, I think we can still
 get an assertion failure when pg_stop_backup(false) waiting for
 archiving is terminated while concurrent an exclusive backup is in
 progress.
>>>
>>> I have just gone through the thread once again, and noticed that it is
>>> actually what I suggested upthread:
>>> https://www.postgresql.org/message-id/cab7npqtm5cdrr5y7yyfky+pvdz6dws_jkg1kstan5m95gam...@mail.gmail.com
>>> But your v2 posted here did not do that so it is incorrect from the start:
>>> https://www.postgresql.org/message-id/cad21aoa+isxyl1_zxmnk9xjhyel5h6rxjttovli7fumqfmc...@mail.gmail.com
>>
>> Sorry, it's my fault. I didn't mean it but I forgot.
>
> My review was wrong as well :)
>
>>> We both got a bit confused here. As do_pg_abort_backup() is only used
>>> for non-exclusive backups (including those taken through the
>>> replication protocol), going through the session lock for checks is
>>> fine. Could you update your patch accordingly please?
>>
>> One question is, since we need to check only the session lock I think
>> that the following change is not necessary. Even if calling
>> CHECK_FOR_INTERRUPTS after set sessionBackupState =
>> SESSION_BACKUP_EXCLUSIVE; we never call do_pg_abort_backup(). Is that
>> right?
>
> Yeah, this one is not strictly necessary for this bug, but it seems to
> me that it would be a good idea for robustness wiht interrupts to be
> consistent with the stop phase when updating the session lock.

Agreed. Attached the updated patch, please review it.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


fix_do_pg_abort_backup_v5.patch
Description: Binary data

Re: [HACKERS] Partition-wise aggregation/grouping

2017-11-15 Thread David Rowley

On 16 November 2017 at 05:57, Konstantin Knizhnik
 wrote:
> The main problem IMHO is that there are a lot of different threads and
> patches related with this topic:(
> And it is very difficult to combine all of them together to achieve the
> final goal: efficient execution of OLAP queries on sharded table.
> It will be nice if somebody who is making the most contribution in this
> direction can somehow maintain it...
> I just faced with particular problem with our pg_shardman extension and now
> (thanks to your patch) I have some working solution for it.
> But certainly I prefer to have this support in mainstream version of
> Postgres.

I don't think it's fair to be asking about additional features on this
thread. It seems to me you're asking about two completely separate
features, with the aim of trying to solve your own problems.

It also looks to me that Jeevan has been clear on what his goals are
for this patch. Perhaps what you're asking for is a logical direction
to travel once this patch is committed, so I think, probably, the best
way to conduct what you're after here is to either:

a) Wait until this is committed and spin up your own thread about
you're proposed changes to allow the PARTIAL aggregate to be pushed
into the foreign server, or;
b) Spin up your own thread now, with reference to this patch as a
prerequisite to your own patch.

I agree that what you're talking about is quite exciting stuff, but
please, let's not talk about it here.

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

Re: [HACKERS] Timeline ID in backup_label file

2017-11-15 Thread David Steele


On 11/15/17 6:01 PM, Michael Paquier wrote:

On Wed, Nov 15, 2017 at 11:16 PM, David Steele  wrote:

Find my review below.

On 10/26/17 2:03 PM, Michael Paquier wrote:


Thanks for the feedback. Attached is a patch to achieve so, I have
added as well a STOP TIMELINE field in the backup history file. Note
that START TIMELINE gets automatically into the backup history file.
Added a CF entry as well.

+   TimeLineID  tli1, tli2;

I'm not that excited about these names but don't have any better ideas.


One is tli_from_segname and the second is tli_from_file. Would
something like that sound better?


Yes, those names are better.


+   if (fscanf(lfp, "START TIMELINE: %u\n", ) == 1)

This didn't work when I tested it (I had intentionally munged the "START
TIMELINE" to produce an error).  The problem is the "START TIME" and
"LABEL" lines which are not being read.  I added a few fgets() calls and
it worked.


Currently backup label files are generated as such:
START WAL LOCATION: 0/228 (file 00010002)
CHECKPOINT LOCATION: 0/260
BACKUP METHOD: streamed
BACKUP FROM: master
START TIME: 2017-11-16 07:52:50 JST
LABEL: popo
START TIMELINE: 1

There could be two things that could be done here:
1) Keep read_backup_label order-dependent and look at the START TIME
and LABEL fields, then kick in ereport(LOG) with the information. This
makes the fields in the backup_label still order-dependent.
2) Make read_backup_label smarter by parsing it line-by-line and fill
in the data wanted. This way the parsing logic and sanity checks are
split into two, and Postgres is smarter at looking at backup_label
files generated by any backup tool.

Which one do you prefer? Getting input from backup tool maintainers is
important here. 2) is more extensible if more fields are added to the
backup_label for a reason or another in the future.


For this patch at least, I think we should do #1.  Getting rid of the 
order dependency is attractive but there may be other programs that are 
depending on the order.  I know you are not proposing to change the 
order now, but it *could* be changed with #2.


Also, I think DEBUG1 would be a more appropriate log level if any 
logging is done.


Regards,
--
-David
da...@pgmasters.net

Re: pspg - psql pager

2017-11-15 Thread Jason Petersen

> On Nov 15, 2017, at 1:41 AM, Pavel Stehule  wrote:
> 
>  I hope so this pager is useful - I know some users who use it few months 
> intensively. But the source code has proof concept quality. It should be 
> cleaned next year.

This is great! I’ve submitted it to homebrew to aid installation for macOS 
users… https://github.com/Homebrew/homebrew-core/pull/20686

--
Jason Petersen
Software Engineer | Citus Data
303.736.9255
ja...@citusdata.com

Re: [HACKERS] Timeline ID in backup_label file

On Wed, Nov 15, 2017 at 11:16 PM, David Steele  wrote:
> Find my review below.
>
> On 10/26/17 2:03 PM, Michael Paquier wrote:
>>
>> Thanks for the feedback. Attached is a patch to achieve so, I have
>> added as well a STOP TIMELINE field in the backup history file. Note
>> that START TIMELINE gets automatically into the backup history file.
>> Added a CF entry as well.
> +   TimeLineID  tli1, tli2;
>
> I'm not that excited about these names but don't have any better ideas.

One is tli_from_segname and the second is tli_from_file. Would
something like that sound better?

> +   if (fscanf(lfp, "START TIMELINE: %u\n", ) == 1)
>
> This didn't work when I tested it (I had intentionally munged the "START
> TIMELINE" to produce an error).  The problem is the "START TIME" and
> "LABEL" lines which are not being read.  I added a few fgets() calls and
> it worked.

Currently backup label files are generated as such:
START WAL LOCATION: 0/228 (file 00010002)
CHECKPOINT LOCATION: 0/260
BACKUP METHOD: streamed
BACKUP FROM: master
START TIME: 2017-11-16 07:52:50 JST
LABEL: popo
START TIMELINE: 1

There could be two things that could be done here:
1) Keep read_backup_label order-dependent and look at the START TIME
and LABEL fields, then kick in ereport(LOG) with the information. This
makes the fields in the backup_label still order-dependent.
2) Make read_backup_label smarter by parsing it line-by-line and fill
in the data wanted. This way the parsing logic and sanity checks are
split into two, and Postgres is smarter at looking at backup_label
files generated by any backup tool.

Which one do you prefer? Getting input from backup tool maintainers is
important here. 2) is more extensible if more fields are added to the
backup_label for a reason or another in the future.
-- 
Michael

Re: Further simplification of c.h's #include section

I wrote:
> Robert Haas  writes:
>> On Wed, Nov 15, 2017 at 4:32 PM, Tom Lane  wrote:
>>> How do you feel about "win32_more.h"?

>> Seems morally equivalent to what you had before.  I think what I would
>> be looking for is a filename that somehow conveys what the difference
>> is between what should go in the existing file and what should go in
>> the new file.  If we don't know, maybe we should find out before we
>> change things.

> Well, the point is whether it gets included before or after the key
> system header files.  "win32_post_headers.h", perhaps?

Actually, on closer inspection, it seems like there's no need for
a windows-specific #include right there at all.  We could move
almost everything that's currently in win32.h to be done in port.h,
at the bottom of c.h rather than at the top.  The only exception
is stuff that would affect #if decisions taken in c.h itself, which
it looks like is only PGDLLIMPORT/PGDLLEXPORT, and those two could
perfectly well be declared before importing system headers.

Now, dropping everything in win32.h into port.h is surely no improvement,
but it seems like we could move all that stuff to a new file
"win32_port.h" and have the bottom of c.h look like

/* Windows-specific compatibility functions */
#if defined(WIN32) || defined(__CYGWIN__)
#include "win32_port.h"
#endif

/* Generic compatibility functions */
#include "port.h"

or else make the new file a sub-include of port.h.

There's also some fair-size stanzas in port.h that could arguably
be moved into win32_port.h if we did it like this.  I think the
parts that are #if WIN32 something #else something-else #endif
are fine as-is, but the parts that are just WIN32 without any
corresponding non-Windows code could be moved.

Thoughts?

regards, tom lane

Re: [HACKERS] [PATCH] Generic type subscripting

2017-11-15 Thread Dmitry Dolgov

> On 14 November 2017 at 22:25, Dmitry Dolgov <9erthali...@gmail.com> wrote:
> But now I wonder if `resnull` is really needed in `jsonb_get_element`,
> `array_get_element` and it seems to me that I can even get rid of it so

On the second thought, no, looks like I'm wrong and it should be like this.
The
reason is that any `fetch` function should be in form

(container, internal) -> extracted value

which means that we need to return an extracted value (for jsonb it's a
`jsonb`,
for array it's an `anyelement`). But at the same time in general case we can
figure out if the result is null only inside a `fetch` function,
(`jsonb_get_element` for jsonb or whatever it may be for a custom data type)
because it returns Datum. So the only way to return this information is by
reference through the `internal` argument. To summarize, If as you said it's
not that critical, I would suggest to leave it as it is.

Re: Updated macOS start scripts

Mark Dilger  writes:
> I am reasonably content with your earlier idea of not keeping support
> for ancient versions of OS X, but if you want to do what you describe
> above, I'd be happy to review that on my 10.6 box to verify that
> everything works at least that far back.  I don't have access to relevant,
> pre-10.4 machines.

IIRC, I already did test it on 10.6.8, but no harm in your double
checking.

regards, tom lane

Re: [HACKERS] Transaction control in procedures

2017-11-15 Thread Peter Eisentraut

On 11/15/17 09:54, Merlin Moncure wrote:
> ... I noticed that:
> *) now() did not advance with commit and,
> *) xact_start via pg_stat_activity did not advance
> 
> Shouldn't both of those advance with the in-loop COMMIT?

I think you are correct.  I'll include that in the next patch version.
It shouldn't be difficult.

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

Re: Updated macOS start scripts


> On Nov 15, 2017, at 1:37 PM, Tom Lane  wrote:
> 
> Mark Dilger  writes:
>> I have tested this on 10.11.6. I had no trouble following the instructions
>> in the README and everything worked as described.
>> As far as I can tell, this patch has not yet been submitted to any 
>> commitfest.
>> I would mark it Read for Committer.
> 
> Thanks for reviewing!  Since you already did review it, I think we can
> probably skip the CF paperwork and just push it.
> 
> One thing that needs to be figured out is what to do with the old osx
> scripts.  My inclination is to remove that subdirectory in HEAD.
> However, I'd like to also stick these new scripts into the supported
> back branches, since the problem is just as real for any back branch
> as it is for v11.  What I propose for the back branches is to add
> contrib/start-scripts/macos/ alongside .../osx/, keeping the latter,
> but adjust its README to note that those scripts are only for pre-10.10
> versions of macOS.

I am reasonably content with your earlier idea of not keeping support
for ancient versions of OS X, but if you want to do what you describe
above, I'd be happy to review that on my 10.6 box to verify that
everything works at least that far back.  I don't have access to relevant,
pre-10.4 machines.

mark

Re: [HACKERS] Transaction control in procedures

2017-11-15 Thread Peter Eisentraut

On 11/8/17 18:48, Simon Riggs wrote:
> What would happen if some of the INSERTs failed? Where would control
> go to? (Maybe this is just "no change" in this particular proc)

An exception is raised and unless the exception is caught (depending on
the PL), control leaves the procedure.  What is already committed stays.

> What happens if the procedure is updated during execution? Presumably
> it keeps executing the original version as seen in the initial
> snapshot?

correct

> Does the xmin of this session advance after each transaction, or do we
> hold the snapshot used for the procedure body open, causing us to hold
> back xmin and prevent vacuuming from being effective?
> 
> What would happen if a procedure recursively called itself? And yet it
> was updated half-way through? Would that throw an error (I think it
> should).

I don't think anything special happens here.  The snapshot that is used
to read the procedure source code and other meta information is released
at a transaction boundary.

>> 3) The PL implementations themselves allocate memory in
>> transaction-bound contexts for convenience as well.  This is usually
>> easy to fix by switching to PortalContext as well.  As you see, the
>> PL/Python code part of the patch is actually very small.  Changes in
>> other PLs would be similar.
> 
> Is there some kind of interlock to prevent dropping the portal half way thru?

I should probably look this up, but I don't think this is fundamentally
different from how VACUUM and CREATE INDEX CONCURRENTLY run inside a
portal and issue multiple transactions in sequence.

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

Re: Further simplification of c.h's #include section

On Wed, Nov 15, 2017 at 4:32 PM, Tom Lane  wrote:
>>> I have no objection to trying to clean things up in that area, but I
>>> request a less-lame filename than win32_2.h
>
>> Sure, if you have a suggestion.
>
> How do you feel about "win32_more.h"?

Seems morally equivalent to what you had before.  I think what I would
be looking for is a filename that somehow conveys what the difference
is between what should go in the existing file and what should go in
the new file.  If we don't know, maybe we should find out before we
change things.

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

Re: Further simplification of c.h's #include section

I wrote:
> Robert Haas  writes:
>> On Wed, Nov 15, 2017 at 10:51 AM, Tom Lane  wrote:
>>> 1. Move what's currently in src/include/port/win32.h (a/k/a
>>> pg_config_os.h) to a new file, say src/include/port/win32_2.h.

>> I have no objection to trying to clean things up in that area, but I
>> request a less-lame filename than win32_2.h

> Sure, if you have a suggestion.

How do you feel about "win32_more.h"?

regards, tom lane

Re: [HACKERS] Inconsistencies between pg_settings and postgresql.conf

On Wed, Sep 13, 2017 at 11:50 AM, Adrian Escoms  wrote:
> I realized that the parameter 'shared_preload_libraries' used to belong to
> category 'Resource Usage / Kernel Resources' but since postgresql version
> 9.4 it was changed in pg_settings to 'Client Connection Defaults / Shared
> Library Preloading' but in postgresql.conf it remains unchanged.
> I attach the updated postgresql.conf.sample.diff with this change.

I think this is a good idea, except I'd leave out this hunk:

@@ -105,7 +105,7 @@


 #--
-# RESOURCE USAGE (except WAL)
+# RESOURCE USAGE
 #--

 # - Memory -

I think a parenthetical gloss is a little different than different
text, and not out of place in what is after all a sample file intended
to be read by humans.

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

Re: Fix number skipping in to_number

I wrote:
> Oliver Ford  writes:
>> On Monday, 13 November 2017, Tom Lane  wrote:
>>> I don't follow your concern?  If "$" is not the correct currency
>>> symbol for the locale, we shouldn't accept it as a match to an L format.
>>> Your patch is tightening what we will accept as a match to a G format,
>>> so I don't see why you're concerned about backward compatibility in
>>> one case but not the other.

>> It's a guess as to the likely use case. I would imagine that people are
>> likely to use a currency symbol different from the locale, but unlikely to
>> use a different group separator. Others might have a different opinion
>> though.

> Well, if they use a currency symbol different from the locale's, they're
> in trouble anyway because the number of bytes might be different.  In most
> encodings, symbols other than "$" are probably not 1-byte characters.
> At the very least I think we need to constrain it enough that it not
> swallow a fractional character.

After more testing I understood your concern about L_currency_symbol:
in C locale that's " ", not "$" as I naively imagined.  Americans,
at least, would be pretty unhappy if "$1234.56" suddenly stopped matching
"L.99".  So it seems like we can't institute a strict matching rule.
However, it is certainly not good that this happens:

regression=# select to_number('1234.56', 'L.99');
 to_number 
---
234.56
(1 row)

To me that seems just as bad as having ',' or 'G' eat a digit.

After some reflection I propose that the rule that we want is:

* ',' and 'G' consume input only if it exactly matches the expected
separator.

* Other non-data template patterns consume a number of input characters
equal to the number of characters they'd produce in output, *except* that
these patterns will not consume data characters (digits, signs, decimal
point, comma).

I think that while we are at it we should take some measures to ensure
that "character" in this definition means "character", not "byte".
It is not good that a euro currency symbol might consume an
encoding-dependent number of input characters.

That leads me to the attached patch.  There is more that could be done
here --- in particular, I'd like to see the character-not-byte-count
rule extended to literal text.  But that seems like fit material for
a different patch.

Also, I noticed that in your form of the patch, the strncmp() could read
past the end of the string, possibly resulting in a crash.  So I made it
use the AMOUNT_TEST infrastructure from NUM_numpart_from_char to avoid that.

One other note: I realized that it was only pure luck that your regression
test cases worked in locales where 'G' is decimal point --- they still
gave the same answer, but through a totally different interpretation of
the input.  That did not seem like a good idea, so I adjusted the
regression test to force C locale for the to_number() tests.  I wish we
could use some other locale here, but then it likely wouldn't be portable
to Windows.

regards, tom lane

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index f901567..35a845c 100644
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
*** SELECT regexp_match('abc01234xyz', '(?:(
*** 5850,5856 
  data based on the given value.  Any text that is not a template pattern is
  simply copied verbatim.  Similarly, in an input template string (for the
  other functions), template patterns identify the values to be supplied by
! the input data string.

--- 5850,5859 
  data based on the given value.  Any text that is not a template pattern is
  simply copied verbatim.  Similarly, in an input template string (for the
  other functions), template patterns identify the values to be supplied by
! the input data string.  If there are characters in the template string
! that are not template patterns, the corresponding characters in the input
! data string are simply skipped over (whether or not they are equal to the
! template string characters).

*** SELECT regexp_match('abc01234xyz', '(?:(
*** 6176,6188 
 Ordinary text is allowed in to_char
 templates and will be output literally.  You can put a substring
 in double quotes to force it to be interpreted as literal text
!even if it contains pattern key words.  For example, in
 '"Hello Year "', the 
 will be replaced by the year data, but the single Y in Year
!will not be.  In to_date, to_number,
!and to_timestamp, double-quoted strings skip the number of
!input characters contained in the string, e.g. "XX"
!skips two input characters.

--- 6179,6193 
 Ordinary text is allowed in to_char
 templates and will be output literally.  You can put a substring
 in double quotes to force it to be interpreted as

Re: [HACKERS] Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)

On Mon, Oct 30, 2017 at 9:19 PM, Badrul Chowdhury  wrote:
> Thank you for the comprehensive review! We are very much in the early stages 
> of contributing to the PG community and we clearly have lots to learn, but we 
> look forward to becoming proficient and active members of the pg community.
>
> Regarding the patch, I have tested it extensively by hand and it works great.

I spent a little more time looking at this patch today.  I think that
the patch should actually send NegotiateProtocolVersion when *either*
the requested version is differs from the latest one we support *or*
an unsupported protocol option is present.  Otherwise, you only find
out about unsupported protocol options if you also request a newer
minor version, which isn't good, because it makes it hard to add new
protocol options *without* bumping the protocol version.

Here's an updated version with that change and a proposed commit message.

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

pgwire-be-rmh-v2.patch
Description: Binary data

Re: [HACKERS] Parallel Hash take II

On Thu, Nov 16, 2017 at 8:09 AM, Robert Haas  wrote:
> On Wed, Nov 15, 2017 at 1:35 PM, Andres Freund  wrote:
>> But this does bug me, and I think it's what made me pause here to make a
>> bad joke.  The way that parallelism treats work_mem makes it even more
>> useless of a config knob than it was before.  Parallelism, especially
>> after this patch, shouldn't compete / be benchmarked against a
>> single-process run with the same work_mem. To make it "fair" you need to
>> compare parallelism against a single threaded run with work_mem *
>> max_parallelism.
>
> I don't really know how to do a fair comparison between a parallel
> plan and a non-parallel plan.  Even if the parallel plan contains zero
> nodes that use work_mem, it might still use more memory than the
> non-parallel plan, because a new backend uses a bunch of memory.  If
> you really want a comparison that is fair on the basis of memory
> usage, you have to take that into account somehow.
>
> But even then, the parallel plan is also almost certainly consuming
> more CPU cycles to produce the same results.  Parallelism is all about
> trading away efficiency for execution time.  Not just because of
> current planner and executor limitations, but intrinsically, parallel
> plans are less efficient.  The globally optimal solution on a system
> that is short on either memory or CPU cycles is to turn parallelism
> off.

The guys who worked on the first attempt at Parallel Query for
Berkeley POSTGRES (and then ripped that out, moving to another project
called XPRS which I have found no trace of, perhaps it finished up in
some commercial RDBMS) wrote this[1]:

"The objective function that XPRS uses for query optimization is a
combination of resource consumption and response time as follows:

  cost = resource consumption + w * response time

Here w is a system-specifc weighting factor. A small w mostly
optimizes resource consumption, while a large w mostly optimizes
response time. Resource consumption is measured by the number of disk
pages accessed and number of tuples processed, while response time is
the elapsed time for executing the query."

http://db.cs.berkeley.edu/papers/ERL-M93-28.pdf

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

Re: [HACKERS] Parallel Hash take II

On Thu, Nov 16, 2017 at 7:57 AM, Peter Geoghegan  wrote:
> The contrast with the situation with Thomas and his hash join patch is
> interesting. Hash join is *much* more sensitive to the availability of
> memory than a sort operation is.
>
>> I don't really have a good answer to "but what should we otherwise do",
>> but I'm doubtful this is quite the right answer.
>
> I think that the work_mem model should be replaced by something that
> centrally budgets memory. It would make sense to be less generous with
> sorts and more generous with hash joins when memory is in short
> supply, for example, and a model like this can make that possible. The
> work_mem model has always forced users to be far too conservative.
> Workloads are very complicated, and always having users target the
> worst case leaves a lot to be desired.

In the old days, Oracle had only simple per-operation memory limits
too, and that applied to every operation in every thread just like our
work_mem.  It's interesting that they had separate knobs for sort and
hash though, and defaulted to giving hash twice as much.

With a whole-plan memory target, our planner would probably begin to
plan join order differently to minimise the number of hash tables in
memory at once, like other RDBMSs.  Not sure how the plan-wide target
should work though -- try many plans, giving different portions of
budget to different subplans?  That should work fine if you like
O(please-melt-my-computer), especially if combined with a similar
approach to choosing worker numbers.  Some kind of feedback system?
Seems like a different kind of planner, but I have no clue.  If you
have ideas/papers/references, it'd be great to see a new thread on
that subject.

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

Re: [HACKERS] Parallel Hash take II

On Thu, Nov 16, 2017 at 7:35 AM, Andres Freund  wrote:
> On 2017-11-15 08:37:11 -0500, Robert Haas wrote:
>> I mean, the very first version of this patch that Thomas submitted was
>> benchmarked by Rafia and had phenomenally good performance
>> characteristics.  That turned out to be because it wasn't respecting
>> work_mem; you can often do a lot better with more memory, and
>> generally you can't do nearly as well with less.  To make comparisons
>> meaningful, they have to be comparisons between algorithms that use
>> the same amount of memory.  And it's not just about testing.  If we
>> add an algorithm that will run twice as fast with equal memory but
>> only allow it half as much, it will probably never get picked and the
>> whole patch is a waste of time.
>
> But this does bug me, and I think it's what made me pause here to make a
> bad joke.  The way that parallelism treats work_mem makes it even more
> useless of a config knob than it was before.  Parallelism, especially
> after this patch, shouldn't compete / be benchmarked against a
> single-process run with the same work_mem. To make it "fair" you need to
> compare parallelism against a single threaded run with work_mem *
> max_parallelism.
>
> Thomas argues that this makes hashjoins be treated faily vis-a-vi
> parallel-oblivious hash join etc. And I think he has somewhat of a
> point. But I don't think it's quite right either: In several of these
> cases the planner will not prefer the multi-process plan because it uses
> more work_mem, it's a cost to be paid. Whereas this'll optimize towards
> using work_mem * max_parallel_workers_per_gather amount of memory.
>
> This makes it pretty much impossible to afterwards tune work_mem on a
> server in a reasonable manner. Previously you'd tune it to something
> like free_server_memory - (max_connections * work_mem *
> 80%_most_complex_query). Which you can't really do anymore now, you'd
> also need to multiply by max_parallel_workers_per_gather. Which means
> that you might end up "forcing" paralellism on a bunch of plans that'd
> normally execute in too short a time to make parallelism worth it.

Currently our way of choosing the number of workers is 'rule based':
we use a simple formula that takes relation sizes and some GUCs and
per-relation options as inputs.  The comparison against non-parallel
plans is cost based of course, but we won't consider any other number
of workers.

Suppose we had 'cost based' worker number selection instead:  simply
try planning with various different worker counts and pick the winner.
Then I think we'd see the moral hazard in this scheme more clearly:
the planner effectively has a free memory printing press.  It will
think that it's a good idea to use a huge number of workers to get
more and more work_mem-sized hash tables or in-memory sorts into
memory (whether that's with partition-wise join, Parallel Hash, or
something else).

We could switch to a model where work_mem is divided by the number of
workers.  Parallel Hash would be able to use a full work_mem by
combining them, and parallel-oblivious Hash would be able to use only
work_mem / participants.  That'd be the other way to give Parallel
Hash a fair amount of memory compared to the competition, but I didn't
propose that because it'd be a change to the already-released
behaviour.  Then I'd have been saying "hey, look at this new plan I
wrote, it performs really well if you first tie the other plans' shoe
laces together".  It may actually be better though, even without
Parallel Hash in the picture.

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

Re: [HACKERS] Issues with logical replication

On Mon, Oct 9, 2017 at 9:19 PM, Stas Kelvich  wrote:
>   I investigated this case and it seems that XactLockTableWait() in 
> SnapBuildWaitSnapshot()
> not always work as expected. XactLockTableWait() waits on LockAcquire() for 
> xid to be
> completed and if we finally got this lock but transaction is still in 
> progress then such xid
> assumed to be a subxid. However LockAcquire/LockRelease cycle can happen 
> after transaction
> set xid, but before XactLockTableInsert().
>
> Namely following history happened for xid 5225 and lead to crash:
>
> [backend] LOG:  AssignTransactionId: XactTopTransactionId = 5225
>[walsender] LOG:  LogCurrentRunningXacts: Wrote RUNNING_XACTS xctn=1, 
> xid[0]=5225
>[walsender] LOG:  XactLockTableWait: LockAcquire 5225
>[walsender] LOG:  XactLockTableWait: LockRelease 5225
> [backend] LOG:  AssignTransactionId: LockAcquire ExclusiveLock 5225
>[walsender] LOG:  TransactionIdIsInProgress: SVC->latestCompletedXid=5224 
> < xid=5225 => true
> [backend] LOG:  CommitTransaction: ProcArrayEndTransaction xid=5225, ipw=0
> [backend] LOG:  CommitTransaction: ResourceOwnerRelease locks xid=5225

Ouch.  This seems like a bug that needs to be fixed, but do you think
it's related to to Petr's proposed fix to set es_output_cid?  That fix
looks reasonable, since we shouldn't try to lock tuples without a
valid CommandId.

Now, having said that, I understand how the lack of that fix could cause:

2017-10-02 18:40:26.101 MSK [2954] ERROR:  attempted to lock invisible tuple

But I do not understand how it could cause:

#3  0x0086ac1d in XactLockTableWait (xid=0, rel=0x0, ctid=0x0,
oper=XLTW_None) at lmgr.c:582

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

Re: Updated macOS start scripts


> On Nov 15, 2017, at 11:00 AM, Tom Lane  wrote:
> 
> Mark Dilger  writes:
>>> On Nov 15, 2017, at 8:32 AM, Tom Lane  wrote:
>>> The stuff in contrib/start-scripts/osx/ does not, as far as I know,
>>> work at all on any recent release of macOS, because SystemStarter
>>> is long gone.  I propose replacing it with the attached, which
>>> I've tested on current-vintage Macs.
> 
>> Overall, I think supporting 10.10 and greater is more relevant than 
>> continuing
>> to support 10.4 and earlier, though I suppose someone could argue for
>> keeping both in side-by-side files with the README directing users which to
>> use for whichever version of OS X they happen to be running.  It's of no
>> consequence to me, since my oldest mac is running 10.6.8, but somebody
>> from the museum of old macs might feel differently.
> 
> Well, we have no buildfarm coverage before 10.4, so to my mind those
> releases are desupported already.  In practice I wonder whether anyone
> is really running PG on such old releases anyway.  Certainly I wouldn't
> think they'd be making new PG installations on machines that old.

I have tested this on 10.11.6. I had no trouble following the instructions
in the README and everything worked as described.

As far as I can tell, this patch has not yet been submitted to any commitfest.
I would mark it Read for Committer.

mark

Re: [HACKERS] pg audit requirements

2017-11-15 16:21 GMT+01:00 David Steele :

> On 11/13/17 1:43 PM, Pavel Stehule wrote:
>
>> 2017-11-13 19:19 GMT+01:00 David Steele >
> >
>
>> Thanks for the input!  I'm not sure this is the best forum for
>> comments, however, since pgAudit is not part of Postgres.
>>
>> Issues can be opened at the github site:
>> https://github.com/pgaudit/pgaudit > audit>
>>
>> I hope so some auditing functionality will be core feature.
>>
>
> Well, that makes two of us!
>
> Have you tried using pgaudit.log_relation?  That would at least get
>> you table name, and schema.  Database and role should really be
>> handled by postgres.  Role is actually pretty tricky - which one
>> should be logged?
>>
>> sure I did it.
>>
>> Who got new rights, who lost rights, new user, dropped user, changes of
>> some features per user (work_mem, logging, ..)
>>
>
> Agreed, the logging for the ROLE class is not very good.  Most detailed
> information is pulled from event triggers which do not fire for global
> objects like roles and databases.
>
> SET operations should be logged with the MISC class, though.
>
> 3. security issues - not enough access rights to database object
>> should be processed and logged in audit log too.
>>
>> Postgres will generate errors on access violations.  Unfortunately,
>> there are currently no hooks that will allow pgAudit to log them.
>>  At least, that I'm aware of.
>>
>> I have a customer, who want to collect all audit data (requires in
>> structured format) and store it to fraud detection software.
>>
>
> You may want to take a look at https://github.com/pgaudit/pgaudit_analyze.
> This a reference implementation that demonstrates how to get pgAudit info
> into a structured form.  It includes logging errors and associating them
> with the statement/transaction that caused the error.
>

thank you for info

>
> I am not sure if one hook helps - It looks so some security related
>> collector (like stats collector or log collector) it is necessary.
>> Currently these informations are too spread over all postgres.
>>
>
> I can't argue with that.
>

I have a patch for pgaudit that does more structured informations to
output, but I waiting to customer to be able to publish it. The my patch
does little bit chaotic result because there are two concepts - using
generic variable - object name from original pgaudit, and using semantic
variables - table name, column name, ... It is not good mix, and when I
have possibility to start again, then probably I'll start from scratch. I
have not any problem with pgaudit design, but two different concepts of
output informations don't work well.

Note: PostgreSQL error systems allows to set additional fields for error
info like table name, column name. Unfortunately, there are not role name.
These fields can be filled by security exceptions and can be simply used by
some like pgaudit applications (without messages parsing)

Regards

Pavel



> --
> -David
> da...@pgmasters.net
>

Re: [HACKERS] Parallel Hash take II

On Wed, Nov 15, 2017 at 1:35 PM, Andres Freund  wrote:
> But this does bug me, and I think it's what made me pause here to make a
> bad joke.  The way that parallelism treats work_mem makes it even more
> useless of a config knob than it was before.  Parallelism, especially
> after this patch, shouldn't compete / be benchmarked against a
> single-process run with the same work_mem. To make it "fair" you need to
> compare parallelism against a single threaded run with work_mem *
> max_parallelism.

I don't really know how to do a fair comparison between a parallel
plan and a non-parallel plan.  Even if the parallel plan contains zero
nodes that use work_mem, it might still use more memory than the
non-parallel plan, because a new backend uses a bunch of memory.  If
you really want a comparison that is fair on the basis of memory
usage, you have to take that into account somehow.

But even then, the parallel plan is also almost certainly consuming
more CPU cycles to produce the same results.  Parallelism is all about
trading away efficiency for execution time.  Not just because of
current planner and executor limitations, but intrinsically, parallel
plans are less efficient.  The globally optimal solution on a system
that is short on either memory or CPU cycles is to turn parallelism
off.

> Thomas argues that this makes hashjoins be treated faily vis-a-vi
> parallel-oblivious hash join etc. And I think he has somewhat of a
> point. But I don't think it's quite right either: In several of these
> cases the planner will not prefer the multi-process plan because it uses
> more work_mem, it's a cost to be paid. Whereas this'll optimize towards
> using work_mem * max_parallel_workers_per_gather amount of memory.

In general, we have no framework for evaluating the global impact on
the system of our decisions.  Not just with parallelism, but in
general, plans that use memory are going to typically beat plans that
don't, because using more memory is a good way to make things run
faster, so the cost goes down, and the cost is what matters.
Everything optimizes for eating as many resources as possible, even if
there is only an extremely marginal gain over a more costly plan that
uses dramatically fewer resources.

A good example of this is a parallel bitmap heap scan vs. a parallel
sequential scan.  With enough workers, we switch from having one
worker scan the index and then having all workers do a joint scan of
the heap -- to just performing a parallel sequential scan.  Because of
Moore's law, as you add workers, waiting for the non-parallel index
scan to build the bitmap eventually looks less desirable than
accepting that you're going to uselessly scan a lot of pages -- you
stop caring, because you have enough raw power to just blast through
it.

The best non-parallel example I can think of off-hand is sorts.
Sometimes, reducing the amount of memory available for a sort really
doesn't cost very much in terms of execution time, but we always run a
sort with the full allotted work_mem, even if that's gigantic.  We
won't use it all if the data is smaller than work_mem, but if there's
batching going on then we will, even if it doesn't really help.

> This makes it pretty much impossible to afterwards tune work_mem on a
> server in a reasonable manner. Previously you'd tune it to something
> like free_server_memory - (max_connections * work_mem *
> 80%_most_complex_query). Which you can't really do anymore now, you'd
> also need to multiply by max_parallel_workers_per_gather. Which means
> that you might end up "forcing" paralellism on a bunch of plans that'd
> normally execute in too short a time to make parallelism worth it.

I think you just need to use max_connections +
Min(max_parallel_workers, max_worker_processes) instead of
max_connections.  You can't use parallel query for every query at the
same time with reasonable settings...

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

Re: [HACKERS] Parallel Hash take II

2017-11-15 Thread Andres Freund

Hi,

On 2017-11-15 10:57:35 -0800, Peter Geoghegan wrote:
> > I don't really have a good answer to "but what should we otherwise do",
> > but I'm doubtful this is quite the right answer.
> 
> I think that the work_mem model should be replaced by something that
> centrally budgets memory. It would make sense to be less generous with
> sorts and more generous with hash joins when memory is in short
> supply, for example, and a model like this can make that possible. The
> work_mem model has always forced users to be far too conservative.
> Workloads are very complicated, and always having users target the
> worst case leaves a lot to be desired.

Obviously that's nice and worthwhile goal, but it seems more than a bit
out of reach for this patchset.

Greetings,

Andres Freund

Re: Updated macOS start scripts

Mark Dilger  writes:
>> On Nov 15, 2017, at 8:32 AM, Tom Lane  wrote:
>> The stuff in contrib/start-scripts/osx/ does not, as far as I know,
>> work at all on any recent release of macOS, because SystemStarter
>> is long gone.  I propose replacing it with the attached, which
>> I've tested on current-vintage Macs.

> Overall, I think supporting 10.10 and greater is more relevant than continuing
> to support 10.4 and earlier, though I suppose someone could argue for
> keeping both in side-by-side files with the README directing users which to
> use for whichever version of OS X they happen to be running.  It's of no
> consequence to me, since my oldest mac is running 10.6.8, but somebody
> from the museum of old macs might feel differently.

Well, we have no buildfarm coverage before 10.4, so to my mind those
releases are desupported already.  In practice I wonder whether anyone
is really running PG on such old releases anyway.  Certainly I wouldn't
think they'd be making new PG installations on machines that old.

regards, tom lane

Re: [HACKERS] Parallel Hash take II

2017-11-15 Thread Peter Geoghegan

On Wed, Nov 15, 2017 at 10:35 AM, Andres Freund  wrote:
>> I realize you're sort of joking here, but I think it's necessary to
>> care about fairness between pieces of code.
>
> Indeed I kinda was.

When I posted v1 of parallel CREATE INDEX, it followed the hash join
model of giving workMem (maintenance_work_mem) to every worker. Robert
suggested that my comparison with a serial case was therefore not
representative, since I was using much more memory. I actually changed
the patch to use a single maintenance_work_mem share for the entire
operation afterwards, which seemed to work better. And, it made very
little difference to performance for my original benchmark in the end,
so I was arguably wasting memory in v1.

>> I mean, the very first version of this patch that Thomas submitted was
>> benchmarked by Rafia and had phenomenally good performance
>> characteristics.  That turned out to be because it wasn't respecting
>> work_mem; you can often do a lot better with more memory, and
>> generally you can't do nearly as well with less.  To make comparisons
>> meaningful, they have to be comparisons between algorithms that use
>> the same amount of memory.  And it's not just about testing.  If we
>> add an algorithm that will run twice as fast with equal memory but
>> only allow it half as much, it will probably never get picked and the
>> whole patch is a waste of time.

The contrast with the situation with Thomas and his hash join patch is
interesting. Hash join is *much* more sensitive to the availability of
memory than a sort operation is.

> I don't really have a good answer to "but what should we otherwise do",
> but I'm doubtful this is quite the right answer.

I think that the work_mem model should be replaced by something that
centrally budgets memory. It would make sense to be less generous with
sorts and more generous with hash joins when memory is in short
supply, for example, and a model like this can make that possible. The
work_mem model has always forced users to be far too conservative.
Workloads are very complicated, and always having users target the
worst case leaves a lot to be desired.

-- 
Peter Geoghegan

Re: [HACKERS] [POC] Faster processing at Gather node

2017-11-15 Thread Andres Freund

Hi,

On 2017-11-15 13:48:18 -0500, Robert Haas wrote:
> I think that we need a little bit deeper analysis here to draw any
> firm conclusions.

Indeed.

> I suspect that one factor is that many of the queries actually send
> very few rows through the Gather.

Yep. I kinda wonder if the same result would present if the benchmarks
were run with parallel_leader_participation. The theory being what were
seing is just that the leader doesn't accept any tuples, and the large
queue size just helps because workers can run for longer.

Greetings,

Andres Freund

Re: [HACKERS] [POC] Faster processing at Gather node

On Tue, Nov 14, 2017 at 7:31 AM, Rafia Sabih
 wrote:
> Case 2: patches applied as in case 1 +
>a) increased PARALLEL_TUPLE_QUEUE_SIZE to 655360
>   No significant change in performance in any query
>b) increased PARALLEL_TUPLE_QUEUE_SIZE to 65536 * 50
>   Performance improved from 20s to 11s for Q12
>c) increased PARALLEL_TUPLE_QUEUE_SIZE to 6553600
>  Q12 shows improvement in performance from 20s to 7s
>
> Case 3: patch applied = faster_gather_v3 as posted at [1]
> Q12 shows improvement in performance from 20s to 8s

I think that we need a little bit deeper analysis here to draw any
firm conclusions.  My own testing showed about a 2x performance
improvement with all 4 patches applied on a query that did a Gather
Merge with many rows.  Now, your testing shows the patches aren't
helping at all.  But what accounts for the difference between your
results?  Without some analysis of that question, this is just a data
point that probably doesn't get us very far.

I suspect that one factor is that many of the queries actually send
very few rows through the Gather.  You didn't send EXPLAIN ANALYZE
outputs for these runs, but I went back and looked at some old tests I
did on a small scale factor and found that, on those tests, Q2, Q6,
Q13, Q14, and Q15 didn't use parallelism at all, while Q1, Q4, Q5, Q7,
Q8, Q9, Q11, Q12, Q19, and Q22 used parallelism, but sent less than
100 rows through Gather.  Obviously, speeding up Gather isn't going to
help at all when only a tiny number of rows are being sent through it.
The remaining seven queries sent the following numbers of rows through
Gather:

3:   ->  Gather Merge  (cost=708490.45..1110533.81
rows=3175456 width=0) (actual time=21932.675..22150.587 rows=118733
loops=1)

10:   ->  Gather Merge  (cost=441168.55..513519.51
rows=574284 width=0) (actual time=15281.370..16319.895 rows=485230
loops=1)

16: ->  Gather
(cost=1000.00..47363.41 rows=297653 width=40) (actual
time=0.414..272.808 rows=297261 loops=1)

17:   ->  Gather  (cost=1000.00..12815.71
rows=2176 width=4) (actual time=2.105..152.966 rows=1943 loops=1)
17: ->  Gather Merge
(cost=2089193.30..3111740.98 rows=7445304 width=0) (actual
time=14071.064..33135.996 rows=9973440 loops=1)

18:   ->  Gather Merge  (cost=3271973.63..7013135.71
rows=29992968 width=0) (actual time=81581.450..81581.594 rows=112
loops=1)

20:   ->  Gather
(cost=1000.00..13368.31 rows=20202 width=4) (actual time=0.361..19.035
rows=21761 loops=1)

21: ->  Gather  (cost=1024178.86..1024179.27
rows=4 width=34) (actual time=12367.266..12377.991 rows=17176 loops=1)

Of those, Q18 is probably uninteresting because it only sends 112
rows, and Q20 and Q16 are probably uninteresting because the Gather
only executed for 19 ms and 272 ms respectively.  Q21 doesn't look
interesting because we ran for 12337.991 seconds and only sent 17176
rows - so the bottleneck is probably generating the tuples, not
sending them.  The places where you'd expect the patch set to help are
where a lot of rows are being sent through the Gather or Gather Merge
node very quickly - so with these plans, probably Q17 is the only that
would have the best chance of going faster with these patches and
maybe Q3 might benefit a bit.

Now obviously your plans are different -- otherwise you couldn't be
seeing a speedup on Q12.  So you have to look at the plans and try to
understand what the big picture is here.  Spending a lot of time
running queries where the time taken by Gather is not the bottleneck
is not a good way to figure out whether we've successfully sped up
Gather.  What would be more useful?  How about:

- Once you've identified the queries where Gather seems like it might
be a bottleneck, run perf without the patch set and see whether Gather
or shm_mq related functions show up high in the profile.  If they do,
run perf which the patch set and see if they become less prominent.

- Try running the test cases that Andres and I tried with and without
the patch set.  See if it helps on those queries.  That will help
verify that your testing procedure is correct, and might also reveal
differences in the effectiveness of that patch set on different
hardware.  You could try this experiment on both PPC and x64, or on
both Linux and MacOS, to see whether CPU architecture and/or operating
system plays a role in the effectiveness of the patch.

I think it's a valid finding that increasing the size of the tuple
queue makes Q12 run faster, but I think that's not because it makes
Gather itself any faster.  Rather, it's because there are fewer
pipeline stalls.  With Gather Merge, whenever a tuple queue becomes
empty, the leader becomes unable to return any more tuples until the
process whose queue is empty generates at least one new tuple.  If
there are multiple

Re: [HACKERS] Parallel tuplesort (for parallel B-Tree index creation)

2017-11-15 Thread Peter Geoghegan

On Tue, Nov 14, 2017 at 10:01 AM, Peter Geoghegan  wrote:
> On Tue, Nov 14, 2017 at 1:41 AM, Rushabh Lathia
>  wrote:
>> Thanks Peter and Thomas for the review comments.
>
> No problem. More feedback:

I see that Robert just committed support for a
parallel_leader_participation GUC. Parallel tuplesort should use this,
too.

It will be easy to adopt the patch to make this work. Just change the
code within nbtsort.c to respect parallel_leader_participation, rather
than leaving that as a compile-time switch. Remove the
force_single_worker variable, and use !parallel_leader_participation
in its place.

The parallel_leader_participation docs will also need to be updated.

-- 
Peter Geoghegan

Re: [HACKERS] Parallel Hash take II

2017-11-15 Thread Andres Freund

On 2017-11-15 08:37:11 -0500, Robert Haas wrote:
> On Tue, Nov 14, 2017 at 4:24 PM, Andres Freund  wrote:
> >> 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 realize you're sort of joking here, but I think it's necessary to
> care about fairness between pieces of code.

Indeed I kinda was.

> I mean, the very first version of this patch that Thomas submitted was
> benchmarked by Rafia and had phenomenally good performance
> characteristics.  That turned out to be because it wasn't respecting
> work_mem; you can often do a lot better with more memory, and
> generally you can't do nearly as well with less.  To make comparisons
> meaningful, they have to be comparisons between algorithms that use
> the same amount of memory.  And it's not just about testing.  If we
> add an algorithm that will run twice as fast with equal memory but
> only allow it half as much, it will probably never get picked and the
> whole patch is a waste of time.

But this does bug me, and I think it's what made me pause here to make a
bad joke.  The way that parallelism treats work_mem makes it even more
useless of a config knob than it was before.  Parallelism, especially
after this patch, shouldn't compete / be benchmarked against a
single-process run with the same work_mem. To make it "fair" you need to
compare parallelism against a single threaded run with work_mem *
max_parallelism.

Thomas argues that this makes hashjoins be treated faily vis-a-vi
parallel-oblivious hash join etc. And I think he has somewhat of a
point. But I don't think it's quite right either: In several of these
cases the planner will not prefer the multi-process plan because it uses
more work_mem, it's a cost to be paid. Whereas this'll optimize towards
using work_mem * max_parallel_workers_per_gather amount of memory.

This makes it pretty much impossible to afterwards tune work_mem on a
server in a reasonable manner. Previously you'd tune it to something
like free_server_memory - (max_connections * work_mem *
80%_most_complex_query). Which you can't really do anymore now, you'd
also need to multiply by max_parallel_workers_per_gather. Which means
that you might end up "forcing" paralellism on a bunch of plans that'd
normally execute in too short a time to make parallelism worth it.

I don't really have a good answer to "but what should we otherwise do",
but I'm doubtful this is quite the right answer.

Greetings,

Andres Freund

Re: Updated macOS start scripts

> On Nov 15, 2017, at 8:32 AM, Tom Lane  wrote:
> 
> The stuff in contrib/start-scripts/osx/ does not, as far as I know,
> work at all on any recent release of macOS, because SystemStarter
> is long gone.  I propose replacing it with the attached, which
> I've tested on current-vintage Macs.
> 

I tested the presence of SystemStarter, which the old implementation
uses, and launchd and launchctl, which your new implementation uses,
on a couple of my mac laptops:

OS X 10.11.6
SystemStarter  - not installed
launchd - installed
launchctl - installed

OS X 10.6.8
SystemStarter - installed
launchd - installed
launchctl - installed

According to Wikipedia:

"SystemStarter appears to have been removed from OS X 10.10 and later 
(reference needed)."

and

"In Mac OS X v10.4, it was deprecated in favor of launchd"

Overall, I think supporting 10.10 and greater is more relevant than continuing
to support 10.4 and earlier, though I suppose someone could argue for
keeping both in side-by-side files with the README directing users which to
use for whichever version of OS X they happen to be running.  It's of no
consequence to me, since my oldest mac is running 10.6.8, but somebody
from the museum of old macs might feel differently.

mark

Re: pspg - psql pager

2017-11-15 18:38 GMT+01:00 Gibrain :

> I can't thank you enough pavel.
> I use psql on a daily basis since 8 years ago, with a lot "less" pain,
> Love vim keybindings and freeze column features.
> Works smoothly in ubuntu 16.04 x86_64.
>
>
> 
>

:)

pspg is postcardware - if you like it, send me postcard

Regards

Pavel

Re: pspg - psql pager

2017-11-15 Thread Gibrain

I can't thank you enough pavel.
I use psql on a daily basis since 8 years ago, with a lot "less" pain,
Love vim keybindings and freeze column features.
Works smoothly in ubuntu 16.04 x86_64.

Re: Updated macOS start scripts


> On Nov 15, 2017, at 8:32 AM, Tom Lane  wrote:
> 
> The stuff in contrib/start-scripts/osx/ does not, as far as I know,
> work at all on any recent release of macOS, because SystemStarter
> is long gone.  I propose replacing it with the attached, which
> I've tested on current-vintage Macs.
> 
> If anyone's interested in reviewing this, I'll stick it into the
> next commitfest.

Sure, I'll take a look.

mark

Re: [HACKERS] Partition-wise aggregation/grouping

2017-11-15 Thread Konstantin Knizhnik

On 15.11.2017 13:35, Jeevan Chalke wrote:

As explained by Ashutosh Bapat in reply
https://www.postgresql.org/message-id/CAFjFpRdpeMTd8kYbM_x0769V-aEKst5Nkg3+coG=8ki7s8z...@mail.gmail.com
we cannot rely on just aggtype==aggtranstype.

Obviously this check (aggtype==aggtranstype) is not correct criteria for
all user defined aggregates.

I just did it as temporary work around for standard aggregates.

However, I have tried pushing partial aggregation over remote server
and also

submitted a PoC patch here:
https://www.postgresql.org/message-id/CAM2+6=uakp9+tsjuh2fbhhwnjc7oyfl1_gvu7mt2fxtvt6g...@mail.gmail.com

I have later removed these patches from Partition-wise-Aggregation
patch set
as it is altogether a different issue than this mail thread. We might
need to

discuss on it separately.

...
Interesting idea of "asynchronous append". However, IMHO it deserves
its own

email-chain.

The main problem IMHO is that there are a lot of different threads and
patches related with this topic:(
And it is very difficult to combine all of them together to achieve the
final goal: efficient execution of OLAP queries on sharded table.
It will be nice if somebody who is making the most contribution in this
direction can somehow maintain it...
I just faced with particular problem with our pg_shardman extension and
now (thanks to your patch) I have some working solution for it.
But certainly I prefer to have this support in mainstream version of
Postgres.

There are two open questions, which I wan to discuss (sorry, may be one
again this is not the right thread for it):

1. Parallel append and FDW/postgres_fdw: should FDW support parallel
scan and do we really need it to support concurrent execution of query
on local and remote partitions?
"Asynchronous append" partly solves this problem, but only for remote
partitions. I do not completely understand all complexity of alternative
approaches.

2. Right now partition-wise aggregation/grouping works only for tables
partitioned using new PG 10 partitioning mechanism. But it doesn't work
for inherited tables, although
there seems to be not so much difference between this two cases. Do you
think that sometimes it will be also supported for standard inheritance
mechanism or there is no sense in it?

--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Updated macOS start scripts

The stuff in contrib/start-scripts/osx/ does not, as far as I know,
work at all on any recent release of macOS, because SystemStarter
is long gone.  I propose replacing it with the attached, which
I've tested on current-vintage Macs.

If anyone's interested in reviewing this, I'll stick it into the
next commitfest.

regards, tom lane

diff --git a/contrib/start-scripts/macos/README b/contrib/start-scripts/macos/README
index ...c4f2d9a .
*** a/contrib/start-scripts/macos/README
--- b/contrib/start-scripts/macos/README
***
*** 0 
--- 1,24 
+ To make macOS automatically launch your PostgreSQL server at system start,
+ do the following:
+ 
+ 1. Edit the postgres-wrapper.sh script and adjust the file path
+ variables at its start to reflect where you have installed Postgres,
+ if that's not /usr/local/pgsql.
+ 
+ 2. Copy the modified postgres-wrapper.sh script into some suitable
+ installation directory.  It can be, but doesn't have to be, where
+ you keep the Postgres executables themselves.
+ 
+ 3. Edit the org.postgresql.postgres.plist file and adjust its path
+ for postgres-wrapper.sh to match what you did in step 2.  Also,
+ if you plan to run the Postgres server under some user name other
+ than "postgres", adjust the UserName parameter value for that.
+ 
+ 4. Copy the modified org.postgresql.postgres.plist file into
+ /Library/LaunchDaemons/.  You must do this as root:
+ sudo cp org.postgresql.postgres.plist /Library/LaunchDaemons
+ because the file will be ignored if it is not root-owned.
+ 
+ At this point a reboot should launch the server.  But if you want
+ to test it without rebooting, you can do
+ sudo launchctl load /Library/LaunchDaemons/org.postgresql.postgres.plist
diff --git a/contrib/start-scripts/macos/org.postgresql.postgres.plist b/contrib/start-scripts/macos/org.postgresql.postgres.plist
index ...fdbd74f .
*** a/contrib/start-scripts/macos/org.postgresql.postgres.plist
--- b/contrib/start-scripts/macos/org.postgresql.postgres.plist
***
*** 0 
--- 1,17 
+ 
+ http://www.apple.com/DTDs/PropertyList-1.0.dtd;>
+ 
+ 
+ 	Label
+ 	org.postgresql.postgres
+ 	ProgramArguments
+ 	
+ 		/bin/sh
+ 		/usr/local/pgsql/bin/postgres-wrapper.sh
+ 	
+ 	UserName
+ 	postgres
+ 	KeepAlive
+ 	
+ 
+ 
diff --git a/contrib/start-scripts/macos/postgres-wrapper.sh b/contrib/start-scripts/macos/postgres-wrapper.sh
index ...3a4ebda .
*** a/contrib/start-scripts/macos/postgres-wrapper.sh
--- b/contrib/start-scripts/macos/postgres-wrapper.sh
***
*** 0 
--- 1,25 
+ #!/bin/sh
+ 
+ # PostgreSQL server start script (launched by org.postgresql.postgres.plist)
+ 
+ # edit these as needed:
+ 
+ # directory containing postgres executable:
+ PGBINDIR="/usr/local/pgsql/bin"
+ # data directory:
+ PGDATA="/usr/local/pgsql/data"
+ # file to receive postmaster's initial log messages:
+ PGLOGFILE="${PGDATA}/pgstart.log"
+ 
+ # (it's recommendable to enable the Postgres logging_collector feature
+ # so that PGLOGFILE doesn't grow without bound)
+ 
+ 
+ # set umask to ensure PGLOGFILE is not created world-readable
+ umask 077
+ 
+ # wait for networking to be up (else server may not bind to desired ports)
+ /usr/sbin/ipconfig waitall
+ 
+ # and launch the server
+ exec "$PGBINDIR"/postgres -D "$PGDATA" >>"$PGLOGFILE" 2>&1

Re: [HACKERS] [PATCH] Incremental sort

2017-11-15 Thread Antonin Houska

Antonin Houska  wrote:

> Alexander Korotkov  wrote:
> 
> > Patch rebased to current master is attached. I'm going to improve my 
> > testing script and post new results. 
> 
> I wanted to review this patch but incremental-sort-8.patch fails to apply. Can
> you please rebase it again?

I could find the matching HEAD quite easily (9b6cb46), so following are my 
comments:

* cost_sort()

** "presorted_keys" missing in the prologue

** when called from label_sort_with_costsize(), 0 is passed for
   "presorted_keys". However label_sort_with_costsize() can sometimes be
   called on an IncrementalSort, in which case there are some "presorted
   keys". See create_mergejoin_plan() for example. (IIUC this should only
   make EXPLAIN inaccurate, but should not cause incorrect decisions.)

** instead of

if (!enable_incrementalsort)
   presorted_keys = false;

you probably meant

if (!enable_incrementalsort)
   presorted_keys = 0;

** instead of

/* Extract presorted keys as list of expressions */
foreach(l, pathkeys)
{
PathKey *key = (PathKey *)lfirst(l);
EquivalenceMember *member = (EquivalenceMember *)
lfirst(list_head(key->pk_eclass->ec_members));

you can use linitial():

/* Extract presorted keys as list of expressions */
foreach(l, pathkeys)
{
PathKey *key = (PathKey *)lfirst(l);
EquivalenceMember *member = (EquivalenceMember *)
linitial(key->pk_eclass->ec_members);

* get_cheapest_fractional_path_for_pathkeys()

The prologue says "... at least partially satisfies the given pathkeys ..."
but I see no change in the function code. In particular the use of
pathkeys_contained_in() does not allow for any kind of partial sorting.

* pathkeys_useful_for_ordering()

Extra whitespace following the comment opening string "/*":

/* 
 * When incremental sort is disabled, pathkeys are useful only when they

* make_sort_from_pathkeys() - the "skipCols" argument should be mentioned in
  the prologue.

* create_sort_plan()

I noticed that pathkeys_common() is called, but the value of n_common_pathkeys
should already be in the path's "skipCols" field if the underlying path is
actually IncrementalSortPath.

* create_unique_plan() does not seem to make use of the incremental
  sort. Shouldn't it do?

* nodeIncrementalSort.c

** These comments seem to contain typos:

"Incremental sort algorithm would sort by xfollowing groups, which have ..."

"Interate while skip cols are same as in saved tuple"

** (This is rather a pedantic comment) I think prepareSkipCols() should be
   defined in front of cmpSortSkipCols().

** the MIN_GROUP_SIZE constant deserves a comment.

* ExecIncrementalSort()

** if (node->tuplesortstate == NULL)

If both branches contain the expression

 node->groupsCount++;

I suggest it to be moved outside the "if" construct.

-- 
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: Further simplification of c.h's #include section

Robert Haas  writes:
> On Wed, Nov 15, 2017 at 10:51 AM, Tom Lane  wrote:
>> 1. Move what's currently in src/include/port/win32.h (a/k/a
>> pg_config_os.h) to a new file, say src/include/port/win32_2.h.

> I have no objection to trying to clean things up in that area, but I
> request a less-lame filename than win32_2.h

Sure, if you have a suggestion.

regards, tom lane

Re: Separate leader buffer info and worker wait info in EXPLAIN output?

On Tue, Nov 14, 2017 at 6:09 PM, Thomas Munro
 wrote:
> EXPLAIN can show per-worker information and aggregated information,
> but doesn't show the leader's information individually.  Should it?
>
> Example:
>
>   Partial Aggregate [...]
> Output: PARTIAL count(*)
> Buffers: shared hit=476
> Worker 0: actual time=5.110..5.110 rows=1 loops=1
>   Buffers: shared hit=150
> Worker 1: actual time=5.088..5.088 rows=1 loops=1
>   Buffers: shared hit=149
>
> Presumably in that case the leader hit 476 - 150 - 149 = 175 shared
> buffers, but why don't we just say so?

This mess is my fault, and I'm happy to have somebody fix it.  The
problem I ran into is what to do about the fields where you can't work
backwards.  When it says actual_time=${FIRST}..${TOTAL}, you can work
out the leader's value for ${TOTAL} by taking the overall total and
subtracting the totals for each worker, but that doesn't work for
${FIRST}.

The problem here is really because of the way
ExecParallelRetrieveInstrumentation works.  That function normally
gets called once per parallel query but could get called multiple
times if workers are relaunched.  Each time it's called, it folds the
data from the most recent set of workers into the leader's information
by calling InstrAggNode().  But because InstrAggNode() isn't fully
reversible, that loses data.  I think what we probably need to do is
modify the definition of PlanState to have an additional
Instrumentation * pointer -- so that know the real leader information
without having to try to work backward.  When I was initially working
on this stuff I was mostly aiming to get done before I ran out of
release cycle and minimize the chances of people asking for parallel
query to be reverted entirely, so I tried to perturb things as little
as possible in this area.  It's probably time to shift the priority
from non-reversion to quality.

> The above is about about increasing visibility of the leader's
> contribution, but it would also be good to be able to see how the
> leader is hurting you by running the plan itself.  Would it make sense
> to instrument the tuple queue interactions so we can show per-worker
> shm_mq_send_bytes wait count and wait time?

Wait count could probably be gathered pretty cheaply, but wait time
would have some significant overhead.  Since we'd presumably only be
doing it in the EXPLAIN ANALYZE case, that wouldn't really be a
problem, but I guess the question is how accurate the results would
be.  If it makes a 50 second query take 100 seconds, then any
percentages you derive from it are probably mostly meaningless.

I think we should have an EXPLAIN option that makes the leader and
every worker run getrusage() before and after the query and include
the results in a separate section of the output.  This would only be
usable with ANALYZE.  Then you could see user/system/elapsed for every
backend involved in the query.  That way, you can see that, for
example, the leader has user approximately equal to elapsed, but for
workers user/elapsed = 20% or whatever.  Granted, it doesn't tell you
WHY processes were off-CPU so it might not be the tuple queue, but I
think it would still be pretty useful -- and it'd be very cheap.

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

Further simplification of c.h's #include section

Yesterday's commit 91aec93e6 got rid of quite a bit of unprincipled cruft
that had accumulated in c.h's #include section.  It occurs to me that we
could clean it up some more, eliminating almost all the Windows-specific
hacking there, by doing this:

1. Move what's currently in src/include/port/win32.h (a/k/a
pg_config_os.h) to a new file, say src/include/port/win32_2.h.

2. Move these bits of c.h into win32.h:

#if defined(_WIN32) && !defined(WIN32)
#define WIN32
#endif

#if _MSC_VER >= 1400 || defined(HAVE_CRTDEFS_H)
#define errcode __msvc_errcode
#include 
#undef errcode
#endif

3. Then the #include for pg_config_os.h just becomes unconditional and
uncluttered.

4. Below the system #includes, where we have

#if defined(WIN32) || defined(__CYGWIN__)
/* We have to redefine some system functions after they are included above. */
#include "pg_config_os.h"
#endif

I'd propose just changing that to include "port/win32_2.h".

Aside from being cleaner, this would provide a clear framework for other
platforms to inject code both before and after the system headers.
To make that happen, we'd have to set up pg_config_os_2.h symlinks and
replace the above-quoted bit with #include "pg_config_os_2.h".  I don't
feel a need to make that happen right now, but it'd be straightforward
to do it if we need to.

Once that's done, there might be reason to rethink the division of
code between win32.h and win32_2.h, but not being a Windows hacker
I'm not qualified to do that.

Thoughts, objections?

regards, tom lane

Re: [HACKERS] moving some partitioning code to executor

On Tue, Nov 14, 2017 at 7:59 PM, Amit Langote
 wrote:
> On 2017/11/15 2:09, Alvaro Herrera wrote:
>> Amit Langote wrote:
>>
>>> Since that 0001 patch was committed [1], here is the rebased patch.  Will
>>> add this to the November commit-fest.
>>
>> Please rebase once more -- 1aba8e651ac3 seems to have broken things
>> in this area pretty thoroughly.
>
> Thanks, done.

Committed.

(Alvaro, hope that's not stepping your toes ...)

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

Re: Rewriting PL/Python's typeio code

Anthony Bykov  writes:
> I have checked your patch. Everything looks fine for me, but I have some 
> doubts:

Thanks for reviewing!

> 1. In file plpy_exec.c there is a typo on line 347:
> "... We use the result and resultin[should be here "g"?] variables...

No, "resultin" is the name of the variable.  Maybe that wasn't a good
choice of name, though --- do you have a better idea?

> 2. In file plpy_cursorobject.c there is a non-volatile variable "elem" used 
> in try-except construction. Is that OK?

Hm, my compiler didn't complain about that.  Did yours?  The variable
is not changed inside the PG_TRY, so according to my ideas of how
this works, it should be OK.  Also, it was like that before, and no
one has reported a problem.

regards, tom lane

Re: [HACKERS] pg audit requirements

2017-11-15 Thread David Steele


On 11/13/17 1:43 PM, Pavel Stehule wrote:
2017-11-13 19:19 GMT+01:00 David Steele 

Thanks for the input!  I'm not sure this is the best forum for
comments, however, since pgAudit is not part of Postgres.

Issues can be opened at the github site:
https://github.com/pgaudit/pgaudit 

I hope so some auditing functionality will be core feature.


Well, that makes two of us!


Have you tried using pgaudit.log_relation?  That would at least get
you table name, and schema.  Database and role should really be
handled by postgres.  Role is actually pretty tricky - which one
should be logged?

sure I did it.

Who got new rights, who lost rights, new user, dropped user, changes of 
some features per user (work_mem, logging, ..)


Agreed, the logging for the ROLE class is not very good.  Most detailed 
information is pulled from event triggers which do not fire for global 
objects like roles and databases.


SET operations should be logged with the MISC class, though.


3. security issues - not enough access rights to database object
should be processed and logged in audit log too.

Postgres will generate errors on access violations.  Unfortunately,
there are currently no hooks that will allow pgAudit to log them. 
At least, that I'm aware of.


I have a customer, who want to collect all audit data (requires in 
structured format) and store it to fraud detection software.


You may want to take a look at 
https://github.com/pgaudit/pgaudit_analyze.  This a reference 
implementation that demonstrates how to get pgAudit info into a 
structured form.  It includes logging errors and associating them with 
the statement/transaction that caused the error.


I am not sure if one hook helps - It looks so some security related 
collector (like stats collector or log collector) it is necessary. 
Currently these informations are too spread over all postgres.


I can't argue with that.

--
-David
da...@pgmasters.net

Re: pspg - psql pager

2017-11-15 Thread Geoff Winkless

On 15 November 2017 at 09:41, Pavel Stehule  wrote:

> Hi
>
> I finished new pager, that can be interesting for postgresql expert users.
>
> It demonstrate some possibilities of TUI and I hope it shows some possible
> directions of future possibilities of psql.
>
> It is available as rpm from our repository or you can get source code from
> github
>
> https://github.com/okbob/pspg
>
> I invite any discussion about this project.
>


Very nice indeed.

Version hasn't pushed to 0.4 (-V still report 0.3-devel).

On RHEL5.3 I get an error on compiling about set_escdelay undefined.
Wasn't a major problem, I just commented that line :)

Geoff

Re: pspg - psql pager

2017-11-15 Thread Nicolas Barbier

2017-11-15 Merlin Moncure :

> I use psql all day, every day.   PSPG IS AWESOME, IF YOU USE PSQL GET
> IT AND DON'T LOOK BACK.
[..]
> Thank you Pavel.

+1!

I especially love it for the improved handling of wide tables (which
are really annoying with “less”). Also --no-mouse is mandatory for me
because I want to be able to copy/paste pieces of text using my
terminal’s mouse handling.

I didn’t find a way yet to get it as a Debian package, but for now I
just compile it manually.

Thanks a lot,

Nicolas

-- 
Nēdırlands: https://baviaan.be/N%C4%93d%C4%B1rlands/

Re: [HACKERS] Transaction control in procedures

2017-11-15 Thread Merlin Moncure

On Wed, Nov 15, 2017 at 7:38 AM, Merlin Moncure  wrote:
> On Tue, Nov 14, 2017 at 5:27 PM, Peter Eisentraut
>>> Can we zero in on this?  The question implied, 'can you do this
>>> without being in a transaction'?  PERFORM do_stuff() is a implicit
>>> transaction, so it ought to end when the function returns right?
>>> Meaning, assuming I was not already in a transaction when hitting this
>>> block, I would not be subject to an endless transaction duration?
>>
>> In the server, you are always in a transaction, so that's not how this
>> works.  I think this also ties into my first response above.
>
> I'll try this out myself, but as long as we can have a *bounded*
> transaction lifetime (basically the time to do stuff + 1 second) via
> something like:
> LOOP
>   
>   COMMIT;
>   PERFORM pg_sleep(1);
> END LOOP;
>
> ... I'm good. I'll try your patch out ASAP.  Thanks for answering all
> my questions.


Trying this out (v2 both patches,  compiled clean, thank you!),
postgres=# CREATE OR REPLACE PROCEDURE foo() AS
$$
BEGIN
  LOOP
PERFORM 1;
COMMIT;
RAISE NOTICE '%', now();
PERFORM pg_sleep(1);
  END LOOP;
END;
$$ LANGUAGE PLPGSQL;
CREATE PROCEDURE
Time: 0.996 ms
postgres=# call foo();
NOTICE:  2017-11-15 08:52:08.936025-06
NOTICE:  2017-11-15 08:52:08.936025-06

... I noticed that:
*) now() did not advance with commit and,
*) xact_start via pg_stat_activity did not advance

Shouldn't both of those advance with the in-loop COMMIT?

merlin

Re: pspg - psql pager

2017-11-15 15:42 GMT+01:00 Merlin Moncure :

> On Wed, Nov 15, 2017 at 4:45 AM, Andreas Joseph Krogh
>  wrote:
> > På onsdag 15. november 2017 kl. 10:41:45, skrev Pavel Stehule
> > :
> >
> > Hi
> >
> > I finished new pager, that can be interesting for postgresql expert
> users.
> > Thanks for making this, "-X --no-mouse" made my day.
>
> This was my first bugfix/suggestion :-D.
>
> I use psql all day, every day.   PSPG IS AWESOME, IF YOU USE PSQL GET
> IT AND DON'T LOOK BACK.   There are no major issues with it; it's
> wonderful.  I do not advise a global definition of PAGER...it will
> break other utilities (like man).  I find the best way to invoke the
> functionality is via bash:
>

:)

you can do it from .psqlrc

\setenv PAGER 'pspg '

or

psql from PostgreSQL 11 should to know PSQL_PAGER

Regards

Pavel


> alias psql='PAGER="/home/mmoncure/pspg -s0 -X --no-mouse" psql'
>
> Thank you Pavel.
>
> merlin
>
>

Re: pspg - psql pager

2017-11-15 Thread Merlin Moncure

On Wed, Nov 15, 2017 at 4:45 AM, Andreas Joseph Krogh
 wrote:
> På onsdag 15. november 2017 kl. 10:41:45, skrev Pavel Stehule
> :
>
> Hi
>
> I finished new pager, that can be interesting for postgresql expert users.
> Thanks for making this, "-X --no-mouse" made my day.

This was my first bugfix/suggestion :-D.

I use psql all day, every day.   PSPG IS AWESOME, IF YOU USE PSQL GET
IT AND DON'T LOOK BACK.   There are no major issues with it; it's
wonderful.  I do not advise a global definition of PAGER...it will
break other utilities (like man).  I find the best way to invoke the
functionality is via bash:

alias psql='PAGER="/home/mmoncure/pspg -s0 -X --no-mouse" psql'

Thank you Pavel.

merlin

Re: Schedule for migration to pglister

2017-11-15 Thread Stephen Frost

Noah,

* Noah Misch (n...@leadboat.com) wrote:
> On Mon, Nov 06, 2017 at 10:36:38AM -0500, Stephen Frost wrote:
> > Our planned migration schedule is as follows:
> > 
> > Nov 6 -
> >   pgsql-www
> > 
> > Nov 13 -
> >   pgsql-hackers
> 
> When each list migrated, its mbox archives stopped receiving new messages:

This was actually anticipated, though we had been thinking that the
migration would be faster and so it wouldn't end up being such a long
time that the mbox's wouldn't get updated.

The plan is to replace those mbox's (which are created by mj2 and then
rsync'd over currently) with ones generated from the archives database,
but that's something which will basically happen to the entire site and
all the lists at once.

Further, there will actually be some differences between the
archive-generated mbox's vs. those that came from mj2; in particular,
the 'date' used by mj2 is the 'received' date (from what I can tell)
while the 'date' in the archives database is the 'Date:' header from
when the message is sent by the user.

That said, this will make the mbox's match the actual webpages, which
they don't currently because the webpages are built from the archive
database while the mbox's are from mj2.  In addition, while trying to
cross-compare the two, I think I'm seeing some cases where mj2 is just
outright dropping messages also (for example, I don't see

https://www.postgresql.org/message-id/20170930224424.ud5ilchmclbl5y5n%40alap3.anarazel.de

anywhere in the mj2 mboxes..).

I'm chatting w/ Magnus about this now and I'm not sure exactly when
we'll end up making the change, but my feeling is that we should either
do it now, or on Monday when we migrate the 'user' lists.  Doesn't seem
to me like we should delay it any longer than that.

Thanks!

Stephen

signature.asc
Description: Digital signature

Re: [HACKERS] Timeline ID in backup_label file

2017-11-15 Thread David Steele

Hi Michael,

Find my review below.

On 10/26/17 2:03 PM, Michael Paquier wrote:
>
> Thanks for the feedback. Attached is a patch to achieve so, I have
> added as well a STOP TIMELINE field in the backup history file. Note
> that START TIMELINE gets automatically into the backup history file.
> Added a CF entry as well.
+   TimeLineID  tli1, tli2;

I'm not that excited about these names but don't have any better ideas.

+   if (fscanf(lfp, "START TIMELINE: %u\n", ) == 1)

This didn't work when I tested it (I had intentionally munged the "START
TIMELINE" to produce an error).  The problem is the "START TIME" and
"LABEL" lines which are not being read.  I added a few fgets() calls and
it worked.

Thanks!
-- 
-David
da...@pgmasters.net

Re: [HACKERS] Transaction control in procedures

2017-11-15 14:38 GMT+01:00 Merlin Moncure :

> On Tue, Nov 14, 2017 at 5:27 PM, Peter Eisentraut
>  wrote:
> > On 11/14/17 16:33, Merlin Moncure wrote:
> >>> One detail in your example is that when you enter the procedure, you
> are
> >>> already in a transaction, so you would have to run either COMMIT or
> >>> ROLLBACK before the START TRANSACTION.
> >>
> >> Ok, that's good, but it seems a little wonky to me to have to issue
> >> COMMIT first.  Shouldn't that be the default?  Meaning you would not
> >> be *in* a transaction unless you specified to be in one.
> >
> > But that's not how this feature is defined in the SQL standard and AFAIK
> > other implementations.  When you enter the procedure call, you are in a
> > transaction.  For one thing, a procedure does not *have* to do
> > transaction control.  So if it's a plain old procedure like a function
> > that just runs a few statements, there needs to be a transaction.
>
> Hm, OK.   Well, SQL Server (which is pretty far from the SQL standard)
> works that way.  See here:
> http://www.4guysfromrolla.com/webtech/080305-1.shtml.  DB2, which is
> very close to the SQL standard, only supports COMMIT/ROLLBACK (not
> begin/start etc)
> https://www.ibm.com/support/knowledgecenter/en/SSULQD_7.2.
> 0/com.ibm.nz.sproc.doc/c_sproc_transaction_commits_and_rollbacks.html.
> Either approach is ok I guess, and always being in a transaction
> probably has some advantages. performance being an obvious one.  With
> DB2, the COMMIT statement acts as kind of a flush, or a paired
> 'commit;begin;'.
>

same in Oracle PL/SQL


> >> Can we zero in on this?  The question implied, 'can you do this
> >> without being in a transaction'?  PERFORM do_stuff() is a implicit
> >> transaction, so it ought to end when the function returns right?
> >> Meaning, assuming I was not already in a transaction when hitting this
> >> block, I would not be subject to an endless transaction duration?
> >
> > In the server, you are always in a transaction, so that's not how this
> > works.  I think this also ties into my first response above.
>
> I'll try this out myself, but as long as we can have a *bounded*
> transaction lifetime (basically the time to do stuff + 1 second) via
> something like:
> LOOP
>   
>   COMMIT;
>   PERFORM pg_sleep(1);
> END LOOP;
>
> ... I'm good. I'll try your patch out ASAP.  Thanks for answering all
> my questions.
>
> merlin
>
>

Re: [HACKERS] A GUC to prevent leader processes from running subplans?

On Wed, Nov 15, 2017 at 8:35 AM, Jesper Pedersen
 wrote:
> Small typo.

Thanks.  That just proves no task is so simple that I can't foul it up.

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

Re: [HACKERS] A GUC to prevent leader processes from running subplans?

On Tue, Nov 14, 2017 at 12:28 AM, Thomas Munro
 wrote:
> Thanks.  You're right.  Rebased and updated to describe what "off" does.

Committed.  I noticed that you didn't add the new GUC to
postgresql.conf.sample, so I did that.  But then I thought it didn't
really belong in the section you put it, so I moved it to
RESOURCES_ASYNCHRONOUS.

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

Logical Replication and triggers

2017-11-15 Thread Thomas Rosenstein

I would like somebody to consider Petr Jelineks patch for worker.c from 
here 
(https://www.postgresql.org/message-id/619c557d-93e6-1833-1692-b010b176ff77%402ndquadrant.com)


I'm was facing the same issue with 10.1 and BEFORE INSERT OR UPDATE 
triggers.


BR
Thomas Rosenstein

Re: [HACKERS] Custom compression methods

On Wed, Nov 15, 2017 at 4:09 AM, Ildus Kurbangaliev
 wrote:
> So in the next version of the patch I can just unlink the options from
> compression methods and dropping compression method will not affect
> already compressed tuples. They still could be decompressed.

I guess I don't understand how that can work.  I mean, if somebody
removes a compression method - i.e. uninstalls the library - and you
don't have a way to make sure there are no tuples that can only be
uncompressed by that library - then you've broken the database.
Ideally, there should be a way to add a new compression method via an
extension ... and then get rid of it and all dependencies thereupon.

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

Re: [HACKERS] Partition-wise aggregation/grouping

2017-11-15 Thread Jeevan Chalke

On Thu, Nov 2, 2017 at 7:36 AM, Robert Haas  wrote:

> On Wed, Nov 1, 2017 at 6:20 PM, Jeevan Chalke
>  wrote:
> > Yep.
> > But as David reported earlier, if we remove the first part i.e. adding
> > cpu_operator_cost per tuple, Merge Append will be preferred over an
> Append
> > node unlike before. And thus, I thought of better having both, but no so
> > sure. Should we remove that part altogether, or add both in a single
> > statement with updated comments?
>
> I was only suggesting that you update the comments.
>

OK. Done in the attached patch set.

I have rebased all my patches on latest HEAD which is at
7518049980be1d90264addab003476ae105f70d4

Thanks


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



-- 
Jeevan Chalke
Technical Architect, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company


partition-wise-agg-v7.tar.gz
Description: GNU Zip compressed data

Re: Schedule for migration to pglister

2017-11-15 Thread Noah Misch

On Mon, Nov 06, 2017 at 10:36:38AM -0500, Stephen Frost wrote:
> Our planned migration schedule is as follows:
> 
> Nov 6 -
>   pgsql-www
> 
> Nov 13 -
>   pgsql-hackers

When each list migrated, its mbox archives stopped receiving new messages:

https://www.postgresql.org/list/pgsql-www/mbox/pgsql-www.201711
https://www.postgresql.org/list/pgsql-hackers/mbox/pgsql-hackers.201711

Re: Rewriting PL/Python's typeio code

2017-11-15 Thread Anthony Bykov

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   not tested
Spec compliant:   not tested
Documentation:tested, passed

Hello,
I have checked your patch. Everything looks fine for me, but I have some doubts:
1. In file plpy_exec.c there is a typo on line 347:
"... We use the result and resultin[should be here "g"?] variables...
2. In file plpy_cursorobject.c there is a non-volatile variable "elem" used in 
try-except construction. Is that OK?

--
Anthony Bykov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

The new status of this patch is: Waiting on Author

RE: User defined data types in Logical Replication

2017-11-15 Thread Huong Dangminh

Hi,

> We are getting the bellow error while trying use Logical Replication with
> user defined data types in a C program (when call elog function).
> 
>  ERROR:  XX000: cache lookup failed for type X
> 

Sorry for continuously disturbing in this topic, but am I missing something 
here?

I mean that in case of type's OID in PUBLICATION host does not exists in 
SUBSCRIPTION host's pg_type, 
it could returns unintended error (the XX000 above) when elog or ereport is 
executed.

For more details, it happen in slot_store_error_callback when it try to call 
format_type_be(localtypoid) for errcontext.
slot_store_error_callback is set in slot_store_cstrings, slot_modify_cstrings 
function and it also be unset here, so the effect here is small but it happens.


---
Thanks and best regards,
Dang Minh Huong
NEC Solution Innovators, Ltd.
http://www.nec-solutioninnovators.co.jp/en/

could not receive data from WAL stream: SSL SYSCALL error: Success

Hi hackers,

I heard a report of an error like this from a user of openssl
1.1.0f-3+deb9u on Debian:

pg_basebackup: could not receive data from WAL stream: SSL SYSCALL
error: Success

I noticed that some man pages for SSL_get_error say this under
SSL_ERROR_SYSCALL:

   Some non-recoverable I/O error occurred.  The OpenSSL error queue
   may contain more information on the error.  For socket I/O on Unix
   systems, consult errno for details.

But others say:

  Some I/O error occurred. The OpenSSL error queue may contain more
  information on the error. If the error queue is empty (i.e. ERR_get_error()
  returns 0), ret can be used to find out more about the error: If ret == 0,
  an EOF was observed that violates the protocol. If ret == -1, the underlying
  BIO reported an I/O error (for socket I/O on Unix systems, consult errno for
  details).

While wondering if it was the documentation or the behaviour that
changed and what it all means, I came across some discussion and a
reverted commit here:

https://github.com/openssl/openssl/issues/1903

The error reported to me seems to have occurred on a release whose man
page *doesn't* describe the ERR_get_error() == 0 case (unlike some of
the earlier tags you can get to from here):

https://github.com/openssl/openssl/blob/OpenSSL_1_1_0-stable/doc/ssl/SSL_get_error.pod

And yet clearly errno didn't hold an error number from a failed
syscall, which seems consistent with the older documented behaviour.

Perhaps pgtls_read(), pgtls_write() and open_client_SSL() should add
"&& ecode != 0" to the if statements in their SSL_ERROR_SYSCALL case
so that this case would fall to the "EOF detected" message instead of
logging the nonsensical (and potentially uninitialised?) errno
message, if indeed this is behaviour described in older releases.  On
the other hand, without documentation to support it in the current
release, we don't really *know* that it's an EOF condition.  Due to
this murkiness and the fact that it's mostly harmless anyway, I'm not
proposing a change, but I thought I'd share this in case it makes more
sense to someone more familiar with this stuff.

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

Re: [HACKERS] Partition-wise aggregation/grouping

2017-11-15 Thread Jeevan Chalke

On Sun, Nov 12, 2017 at 1:59 AM, Konstantin Knizhnik <
k.knizh...@postgrespro.ru> wrote:

> On 10/27/2017 02:01 PM, Jeevan Chalke wrote:
>
>> Hi,
>>
>> Attached new patch-set here. Changes include:
>>
>> 1. Added separate patch for costing Append node as discussed up-front in
>> the
>> patch-set.
>> 2. Since we now cost Append node, we don't need
>> partition_wise_agg_cost_factor
>> GUC. So removed that. The remaining patch hence merged into main
>> implementation
>> patch.
>> 3. Updated rows in test-cases so that we will get partition-wise plans.
>>
>> Thanks
>>
>
> I applied partition-wise-agg-v6.tar.gz patch to  the master and use
> shard.sh example from https://www.postgresql.org/mes
> sage-id/14577.1509723225%40localhost
> Plan for count(*) is the following:
>
> shard=# explain select count(*) from orders;
>   QUERY PLAN
> 
> ---
>  Finalize Aggregate  (cost=100415.29..100415.30 rows=1 width=8)
>->  Append  (cost=50207.63..100415.29 rows=2 width=8)
>  ->  Partial Aggregate  (cost=50207.63..50207.64 rows=1 width=8)
>->  Foreign Scan on orders_0 (cost=101.00..50195.13
> rows=5000 width=0)
>  ->  Partial Aggregate  (cost=50207.63..50207.64 rows=1 width=8)
>->  Foreign Scan on orders_1 (cost=101.00..50195.13
> rows=5000 width=0)
>
>
> We really calculate partial aggregate for each partition, but to do we
> still have to fetch all data from remote host.
> So for foreign partitions such plans is absolutely inefficient.
> Amy be it should be combined with some other patch?
> For example, with  agg_pushdown_v4.tgz patch
> https://www.postgresql.org/message-id/14577.1509723225%40localhost ?
> But it is not applied after partition-wise-agg-v6.tar.gz patch.
> Also postgres_fdw in 11dev is able to push down aggregates without
> agg_pushdown_v4.tgz patch.
>
> In 0009-Teach-postgres_fdw-to-push-aggregates-for-child-rela.patch
> there is the following check:
>
>  /* Partial aggregates are not supported. */
> +   if (extra->isPartial)
> +   return;
>
> If we just comment this line then produced plan will be the following:
>
> shard=# explain select sum(product_id) from orders;
>QUERY PLAN
> 
>  Finalize Aggregate  (cost=308.41..308.42 rows=1 width=8)
>->  Append  (cost=144.18..308.41 rows=2 width=8)
>  ->  Foreign Scan  (cost=144.18..154.20 rows=1 width=8)
>Relations: Aggregate on (public.orders_0 orders)
>  ->  Foreign Scan  (cost=144.18..154.20 rows=1 width=8)
>Relations: Aggregate on (public.orders_1 orders)
> (6 rows)
>
> And it is actually desired plan!
> Obviously such approach will not always work. FDW really doesn't support
> partial aggregates now.
> But for most frequently used aggregates: sum, min, max, count
> aggtype==aggtranstype and there is no difference
> between partial and normal aggregate calculation.
> So instead of (extra->isPartial) condition we can add more complex check
> which will traverse pathtarget expressions and
> check if it can be evaluated in this way. Or... extend FDW API to support
> partial aggregation.
>

As explained by Ashutosh Bapat in reply
https://www.postgresql.org/message-id/CAFjFpRdpeMTd8kYbM_x0769V-aEKst5Nkg3+coG=8ki7s8z...@mail.gmail.com
we cannot rely on just aggtype==aggtranstype.

However, I have tried pushing partial aggregation over remote server and
also
submitted a PoC patch here:
https://www.postgresql.org/message-id/CAM2+6=uakp9+tsjuh2fbhhwnjc7oyfl1_gvu7mt2fxtvt6g...@mail.gmail.com

I have later removed these patches from Partition-wise-Aggregation patch set
as it is altogether a different issue than this mail thread. We might need
to
discuss on it separately.


>
> But even the last plan is not ideal: it will calculate predicates at each
> remote node sequentially.
> There is parallel append patch:
> https://www.postgresql.org/message-id/CAJ3gD9ctEcrVUmpY6fq_J
> UB6WDKGXAGd70EY68jVFA4kxMbKeQ%40mail.gmail.com
> but ... FDW doesn't support parallel scan, so parallel append can not be
> applied in this case.
> And we actually do not need parallel append with all its dynamic workers
> here.
> We just need to start commands at all remote servers and only after it
> fetch results (which can be done sequentially).
>
> I am investigating problem of efficient execution of OLAP queries on
> sharded tables (tables with remote partitions).
> After reading all this threads and corresponding  patches, it seems to me
> that we already have most of parts of the puzzle, what we need is to put
> them on right places and may be add missed ones.
> I wonder if somebody is busy with it and can I somehow help here?
>
> Also I am not quite sure about the best approach with parallel execution
> of distributed query at all nodes.
> Should we make postgres_fdw

Re: [HACKERS] Transform for pl/perl

2017-11-15 Thread Aleksander Alekseev

Hello, hackers.

> > I'm curious, how much benefit we could get from this ? There are several
> > publicly available json datasets, which can be used to measure performance
> > gaining. I have bookmarks and review datasets available from
> > http://www.sai.msu.su/~megera/postgres/files/, look at js.dump.gz and
> > jr.dump.gz
> >
> 
> I don't expect significant performance effect - it remove some
> transformations - perl object -> json | json -> jsonb - but on modern cpu
> these transformations should be fast. For me - main benefit is user comfort
> - it does direct transformation from perl object -> jsonb

I completely agree that currently the main benefit of this feature is
user comfort. So correctness is the priority. When we make sure that the
implementation is correct we can start worry about the performance.
Probably in a separate patch.

Thanks for the datasets though!

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Transform for pl/perl

2017-11-15 10:24 GMT+01:00 Oleg Bartunov :

>
>
> On 14 Nov 2017 11:35, "Anthony Bykov"  wrote:
>
> On Fri, 10 Nov 2017 14:40:21 +0100
> Pavel Stehule  wrote:
>
> > Hi
> >
> > 2017-10-24 14:27 GMT+02:00 Anthony Bykov :
> >
> > > There are some moments I should mention:
> > > 1. {"1":1}::jsonb is transformed into HV {"1"=>"1"}, while
> > > ["1","2"]::jsonb is transformed into AV ["1", "2"]
> > >
> > > 2. If there is a numeric value appear in jsonb, it will be
> > > transformed to SVnv through string (Numeric->String->SV->SVnv). Not
> > > the best solution, but as far as I understand this is usual
> > > practise in postgresql to serialize Numerics and de-serialize them.
> > >
> > > 3. SVnv is transformed into jsonb through string
> > > (SVnv->String->Numeric).
> > >
> > > An example may also be helpful to understand extension. So, as an
> > > example, function "test" transforms incoming jsonb into perl,
> > > transforms it back into jsonb and returns it.
> > >
> > > create extension jsonb_plperl cascade;
> > >
> > > create or replace function test(val jsonb)
> > > returns jsonb
> > > transform for type jsonb
> > > language plperl
> > > as $$
> > > return $_[0];
> > > $$;
> > >
> > > select test('{"1":1,"example": null}'::jsonb);
> > >
> > >
> > I am looking to this patch:
> >
> > 1. the patch contains some artefacts - look the word "hstore"
> >
> > 2. I got lot of warnings
> >
> >
> > make[1]: Vstupuje se do adresáře
> > „/home/pavel/src/postgresql/contrib/jsonb_plperl“
> > gcc -Wall -Wmissing-prototypes -Wpointer-arith
> > -Wdeclaration-after-statement -Wendif-labels
> > -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
> > -fwrapv -fexcess-precision=standard -g -ggdb -Og -g3
> > -fno-omit-frame-pointer -fPIC -I../../src/pl/plperl -I. -I.
> > -I../../src/include  -D_GNU_SOURCE -I/usr/include/libxml2
> > -I/usr/lib64/perl5/CORE  -c -o jsonb_plperl.o jsonb_plperl.c
> > jsonb_plperl.c: In function ‘SV_FromJsonbValue’: jsonb_plperl.c:83:9:
> > warning: ‘result’ may be used uninitialized in this function
> > [-Wmaybe-uninitialized] return (result);
> >  ^
> > jsonb_plperl.c: In function ‘SV_FromJsonb’:
> > jsonb_plperl.c:95:10: warning: ‘object’ may be used uninitialized in
> > this function [-Wmaybe-uninitialized]
> >   HV *object;
> >   ^~
> > In file included from /usr/lib64/perl5/CORE/perl.h:5644:0,
> >  from ../../src/pl/plperl/plperl.h:52,
> >  from jsonb_plperl.c:17:
> > /usr/lib64/perl5/CORE/embed.h:404:19: warning: ‘value’ may be used
> > uninitialized in this function [-Wmaybe-uninitialized]
> >  #define newRV(a)  Perl_newRV(aTHX_ a)
> >^~
> > jsonb_plperl.c:101:10: note: ‘value’ was declared here
> >   SV *value;
> >   ^
> > gcc -Wall -Wmissing-prototypes -Wpointer-arith
> > -Wdeclaration-after-statement -Wendif-labels
> > -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
> > -fwrapv -fexcess-precision=standard -g -ggdb -Og -g3
> > -fno-omit-frame-pointer -fPIC -shared -o jsonb_plperl.so
> > jsonb_plperl.o  -L../../src/port -L../../src/common -Wl,--as-needed
> > -Wl,-rpath,'/usr/lib64/perl5/CORE',--enable-new-dtags   -Wl,-z,relro
> > -specs=/usr/lib/rpm/redhat/redhat-hardened-ld
> > -fstack-protector-strong -L/usr/local/lib  -L/usr/lib64/perl5/CORE
> > -lperl -lpthread -lresolv -lnsl -ldl -lm -lcrypt -lutil -lc make[1]:
> > Opouští se adresář „/home/pavel/src/postgresql/contrib/jsonb_plperl“
> >
> > [pavel@nemesis contrib]$ gcc --version
> > gcc (GCC) 7.2.1 20170915 (Red Hat 7.2.1-2)
> > Copyright (C) 2017 Free Software Foundation, Inc.
> > This is free software; see the source for copying conditions.  There
> > is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A
> > PARTICULAR PURPOSE.
> >
> > 3. regress tests passed
> >
> > 4. There are not any documentation - probably it should be part of
> > PLPerl
> >
> > 5. The regress tests doesn't coverage other datatypes than numbers. I
> > miss boolean, binary, object, ... Maybe using data::dumper or some
> > similar can be interesting
> >
> > Note - it is great extension, I am pleasured so transformations are
> > used.
> >
> > Regards
> >
> > Pavel
> > >
> > > --
> > > Sent via pgsql-hackers mailing list (pgsql-hack...@postgresql.org)
> > > To make changes to your subscription:
> > > http://www.postgresql.org/mailpref/pgsql-hackers
> > >
>
> Hello,
> Thank you for your review. I have fixed most of your comments, except
> for the 5-th part, about data::dumper - I just couldn't understand
> your point, but I've added more tests with more complex objects if this
> helps.
>
> Please, take a look at new patch. You can find it in attachments to
> this message (it is called "0001-jsonb_plperl-extension-v2.patch")
>
>
> I'm curious, how much benefit we could get from this ? There are several
> publicly available json datasets, which

Re: [HACKERS] Transform for pl/perl

2017-11-15 Thread Oleg Bartunov

On 14 Nov 2017 11:35, "Anthony Bykov"  wrote:

On Fri, 10 Nov 2017 14:40:21 +0100
Pavel Stehule  wrote:

> Hi
>
> 2017-10-24 14:27 GMT+02:00 Anthony Bykov :
>
> > There are some moments I should mention:
> > 1. {"1":1}::jsonb is transformed into HV {"1"=>"1"}, while
> > ["1","2"]::jsonb is transformed into AV ["1", "2"]
> >
> > 2. If there is a numeric value appear in jsonb, it will be
> > transformed to SVnv through string (Numeric->String->SV->SVnv). Not
> > the best solution, but as far as I understand this is usual
> > practise in postgresql to serialize Numerics and de-serialize them.
> >
> > 3. SVnv is transformed into jsonb through string
> > (SVnv->String->Numeric).
> >
> > An example may also be helpful to understand extension. So, as an
> > example, function "test" transforms incoming jsonb into perl,
> > transforms it back into jsonb and returns it.
> >
> > create extension jsonb_plperl cascade;
> >
> > create or replace function test(val jsonb)
> > returns jsonb
> > transform for type jsonb
> > language plperl
> > as $$
> > return $_[0];
> > $$;
> >
> > select test('{"1":1,"example": null}'::jsonb);
> >
> >
> I am looking to this patch:
>
> 1. the patch contains some artefacts - look the word "hstore"
>
> 2. I got lot of warnings
>
>
> make[1]: Vstupuje se do adresáře
> „/home/pavel/src/postgresql/contrib/jsonb_plperl“
> gcc -Wall -Wmissing-prototypes -Wpointer-arith
> -Wdeclaration-after-statement -Wendif-labels
> -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
> -fwrapv -fexcess-precision=standard -g -ggdb -Og -g3
> -fno-omit-frame-pointer -fPIC -I../../src/pl/plperl -I. -I.
> -I../../src/include  -D_GNU_SOURCE -I/usr/include/libxml2
> -I/usr/lib64/perl5/CORE  -c -o jsonb_plperl.o jsonb_plperl.c
> jsonb_plperl.c: In function ‘SV_FromJsonbValue’: jsonb_plperl.c:83:9:
> warning: ‘result’ may be used uninitialized in this function
> [-Wmaybe-uninitialized] return (result);
>  ^
> jsonb_plperl.c: In function ‘SV_FromJsonb’:
> jsonb_plperl.c:95:10: warning: ‘object’ may be used uninitialized in
> this function [-Wmaybe-uninitialized]
>   HV *object;
>   ^~
> In file included from /usr/lib64/perl5/CORE/perl.h:5644:0,
>  from ../../src/pl/plperl/plperl.h:52,
>  from jsonb_plperl.c:17:
> /usr/lib64/perl5/CORE/embed.h:404:19: warning: ‘value’ may be used
> uninitialized in this function [-Wmaybe-uninitialized]
>  #define newRV(a)  Perl_newRV(aTHX_ a)
>^~
> jsonb_plperl.c:101:10: note: ‘value’ was declared here
>   SV *value;
>   ^
> gcc -Wall -Wmissing-prototypes -Wpointer-arith
> -Wdeclaration-after-statement -Wendif-labels
> -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
> -fwrapv -fexcess-precision=standard -g -ggdb -Og -g3
> -fno-omit-frame-pointer -fPIC -shared -o jsonb_plperl.so
> jsonb_plperl.o  -L../../src/port -L../../src/common -Wl,--as-needed
> -Wl,-rpath,'/usr/lib64/perl5/CORE',--enable-new-dtags   -Wl,-z,relro
> -specs=/usr/lib/rpm/redhat/redhat-hardened-ld
> -fstack-protector-strong -L/usr/local/lib  -L/usr/lib64/perl5/CORE
> -lperl -lpthread -lresolv -lnsl -ldl -lm -lcrypt -lutil -lc make[1]:
> Opouští se adresář „/home/pavel/src/postgresql/contrib/jsonb_plperl“
>
> [pavel@nemesis contrib]$ gcc --version
> gcc (GCC) 7.2.1 20170915 (Red Hat 7.2.1-2)
> Copyright (C) 2017 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions.  There
> is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A
> PARTICULAR PURPOSE.
>
> 3. regress tests passed
>
> 4. There are not any documentation - probably it should be part of
> PLPerl
>
> 5. The regress tests doesn't coverage other datatypes than numbers. I
> miss boolean, binary, object, ... Maybe using data::dumper or some
> similar can be interesting
>
> Note - it is great extension, I am pleasured so transformations are
> used.
>
> Regards
>
> Pavel
> >
> > --
> > Sent via pgsql-hackers mailing list (pgsql-hack...@postgresql.org)
> > To make changes to your subscription:
> > http://www.postgresql.org/mailpref/pgsql-hackers
> >

Hello,
Thank you for your review. I have fixed most of your comments, except
for the 5-th part, about data::dumper - I just couldn't understand
your point, but I've added more tests with more complex objects if this
helps.

Please, take a look at new patch. You can find it in attachments to
this message (it is called "0001-jsonb_plperl-extension-v2.patch")


I'm curious, how much benefit we could get from this ? There are several
publicly available json datasets, which can be used to measure performance
gaining. I have bookmarks and review datasets available from
http://www.sai.msu.su/~megera/postgres/files/, look at js.dump.gz and
jr.dump.gz


--
Anthony Bykov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

no library dependency in Makefile?

2017-11-15 Thread 高增琦

Hi,

Recently, I found 'psql' is not rebuilt automatically after
editing 'fe_utils/psqlscan.l'.

The detail is:
'psqlscan.l' is part of 'libpgfeutils.a' which will be built
into 'psql' statically. But there is no dependency rule between
them.

It's OK for 'libpq' since 'libpq' is a dynamic library.
For a static library such as 'libpgfeutils.a', should we
add dependency rule in Makefile?

-- 
GaoZengqi
pgf...@gmail.com
zengqi...@gmail.com

Re: [HACKERS] Transform for pl/perl

Hi


> Hello,
> Thank you for your review. I have fixed most of your comments, except
> for the 5-th part, about data::dumper - I just couldn't understand
> your point, but I've added more tests with more complex objects if this
> helps.
>
> Please, take a look at new patch. You can find it in attachments to
> this message (it is called "0001-jsonb_plperl-extension-v2.patch")
>

I changed few lines in regress tests.

Now, I am think so this patch is ready for commiters.

1. all tests passed
2. there are some basic documentation

I have not any other objections

Regards

Pavel


> --
> Anthony Bykov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>
diff --git a/contrib/Makefile b/contrib/Makefile
index 8046ca4f39..53d44fe4a6 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -75,9 +75,9 @@ ALWAYS_SUBDIRS += sepgsql
 endif
 
 ifeq ($(with_perl),yes)
-SUBDIRS += hstore_plperl
+SUBDIRS += hstore_plperl jsonb_plperl
 else
-ALWAYS_SUBDIRS += hstore_plperl
+ALWAYS_SUBDIRS += hstore_plperl jsonb_plperl
 endif
 
 ifeq ($(with_python),yes)
diff --git a/contrib/jsonb_plperl/Makefile b/contrib/jsonb_plperl/Makefile
new file mode 100644
index 00..8c427c545a
--- /dev/null
+++ b/contrib/jsonb_plperl/Makefile
@@ -0,0 +1,40 @@
+# contrib/jsonb_plperl/Makefile
+
+MODULE_big = jsonb_plperl
+OBJS = jsonb_plperl.o $(WIN32RES)
+PGFILEDESC = "jsonb_plperl - jsonb transform for plperl"
+
+PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plperl
+
+EXTENSION = jsonb_plperlu jsonb_plperl
+DATA = jsonb_plperlu--1.0.sql jsonb_plperl--1.0.sql
+
+REGRESS = jsonb_plperl jsonb_plperlu
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = contrib/jsonb_plperl
+top_builddir = ../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
+
+# We must link libperl explicitly
+ifeq ($(PORTNAME), win32)
+# these settings are the same as for plperl
+override CPPFLAGS += -DPLPERL_HAVE_UID_GID -Wno-comment
+# ... see silliness in plperl Makefile ...
+SHLIB_LINK += $(sort $(wildcard ../../src/pl/plperl/libperl*.a))
+else
+rpathdir = $(perl_archlibexp)/CORE
+SHLIB_LINK += $(perl_embed_ldflags)
+endif
+
+# As with plperl we need to make sure that the CORE directory is included
+# last, probably because it sometimes contains some header files with names
+# that clash with some of ours, or with some that we include, notably on
+# Windows.
+override CPPFLAGS := $(CPPFLAGS) $(perl_embed_ccflags) -I$(perl_archlibexp)/CORE
diff --git a/contrib/jsonb_plperl/expected/jsonb_plperl.out b/contrib/jsonb_plperl/expected/jsonb_plperl.out
new file mode 100644
index 00..fdaa4d013b
--- /dev/null
+++ b/contrib/jsonb_plperl/expected/jsonb_plperl.out
@@ -0,0 +1,197 @@
+CREATE EXTENSION jsonb_plperl CASCADE;
+NOTICE:  installing required extension "plperl"
+-- test hash -> jsonb
+CREATE FUNCTION testHVToJsonb() RETURNS jsonb
+LANGUAGE plperl
+TRANSFORM FOR TYPE jsonb
+AS $$
+$val = {a => 1, b => 'boo', c => undef};
+return $val;
+$$;
+SELECT testHVToJsonb();
+  testhvtojsonb  
+-
+ {"a": 1, "b": "boo", "c": null}
+(1 row)
+
+-- test array -> jsonb
+CREATE FUNCTION testAVToJsonb() RETURNS jsonb
+LANGUAGE plperl
+TRANSFORM FOR TYPE jsonb
+AS $$
+$val = [{a => 1, b => 'boo', c => undef}, {d => 2}];
+return $val;
+$$;
+SELECT testAVToJsonb();
+testavtojsonb
+-
+ [{"a": 1, "b": "boo", "c": null}, {"d": 2}]
+(1 row)
+
+-- test scalar -> jsonb
+CREATE FUNCTION testSVToJsonb() RETURNS jsonb
+LANGUAGE plperl
+TRANSFORM FOR TYPE jsonb
+AS $$
+$val = 1;
+return $val;
+$$;
+SELECT testSVToJsonb();
+ testsvtojsonb 
+---
+ 1
+(1 row)
+
+-- test jsonb -> scalar -> jsonb
+CREATE FUNCTION testSVToJsonb2(val jsonb) RETURNS jsonb
+LANGUAGE plperl
+TRANSFORM FOR TYPE jsonb
+AS $$
+return $_[0];
+$$;
+SELECT testSVToJsonb2('null');
+ testsvtojsonb2 
+
+ null
+(1 row)
+
+SELECT testSVToJsonb2('1');
+ testsvtojsonb2 
+
+ 1
+(1 row)
+
+SELECT testSVToJsonb2('-1');
+ testsvtojsonb2 
+
+ -1
+(1 row)
+
+SELECT testSVToJsonb2('1.2');
+ testsvtojsonb2 
+
+ 1.2
+(1 row)
+
+SELECT testSVToJsonb2('-1.2');
+ testsvtojsonb2 
+
+ -1.2
+(1 row)
+
+SELECT testSVToJsonb2('"string"');
+ testsvtojsonb2 
+
+ "string"
+(1 row)
+
+-- type information is lost inside
+SELECT testSVToJsonb2('true');
+ testsvtojsonb2 
+
+ 1
+(1 row)
+
+SELECT testSVToJsonb2('false');
+ testsvtojsonb2 
+
+ 0
+(1 row)
+
+SELECT testSVToJsonb2('[]');
+ testsvtojsonb2 
+
+ []
+(1 row)
+
+SELECT testSVToJsonb2('[null,null]');
+ testsvtojsonb2 
+
+ [null, null]
+(1 row)
+
+SELECT testSVToJsonb2('[1,2,3]');
+ testsvtojsonb2 
+
+ [1, 2, 3]
+(1 row)
+
+SELECT testSVToJsonb2('[-1,2,-3]');
+

Re: [HACKERS] [PATCH] Add recovery_min_apply_delay_reconnect recovery option