Re: [HACKERS] [BUGS] Bug in Physical Replication Slots (at least 9.5)?

[Kyotaro HORIGUCHI]

Hello,

At Thu, 30 Mar 2017 15:59:14 +1100, Venkata B Nagothi  wrote 
in 

[HACKERS] Somebody has not thought through subscription locking considerations

[Tom Lane]

I noticed this failure report:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dangomushi=2017-03-29%2019%3A45%3A27

in which we find

*** 
/home/buildfarm/data/buildroot/HEAD/pgsql.build/src/test/regress/expected/updatable_views.out
   Thu Mar 30 04:45:43 2017
--- 
/home/buildfarm/data/buildroot/HEAD/pgsql.build/src/test/regress/results/updatable_views.out
Thu Mar 30 05:32:37 2017
***
*** 349,354 
--- 349,358 
  DROP VIEW ro_view10, ro_view12, ro_view18;
  DROP SEQUENCE seq CASCADE;
  NOTICE:  drop cascades to view ro_view19
+ ERROR:  deadlock detected
+ DETAIL:  Process 7576 waits for AccessShareLock on relation 1259 of database 
16384; blocked by process 7577.
+ Process 7577 waits for ShareRowExclusiveLock on relation 6102 of database 
16384; blocked by process 7576.
+ HINT:  See server log for query details.
  -- simple updatable view
  CREATE TABLE base_tbl (a int PRIMARY KEY, b text DEFAULT 'Unspecified');
  INSERT INTO base_tbl SELECT i, 'Row ' || i FROM generate_series(-2, 2) g(i);

and the referenced bit of log is

[58dc19dd.1d98:175] ERROR:  deadlock detected
[58dc19dd.1d98:176] DETAIL:  Process 7576 waits for AccessShareLock on relation 
1259 of database 16384; blocked by process 7577.
Process 7577 waits for ShareRowExclusiveLock on relation 6102 of 
database 16384; blocked by process 7576.
Process 7576: DROP SEQUENCE seq CASCADE;
Process 7577: VACUUM FULL pg_class;
[58dc19dd.1d98:177] HINT:  See server log for query details.
[58dc19dd.1d98:178] STATEMENT:  DROP SEQUENCE seq CASCADE;

Of course, 1259 is pg_class and 6102 is pg_subscription_rel.

I await with interest an explanation of what "VACUUM FULL pg_class" is
doing trying to acquire ShareRowExclusiveLock on pg_subscription_rel, not
to mention why a DROP SEQUENCE is holding some fairly strong lock on that
relation.  *Especially* in a situation where no subscriptions exist ---
but even if any did, this seems unacceptable on its face.  Access to core
catalogs like pg_class cannot depend on random other stuff.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Partition-wise join for join between (declaratively) partitioned tables

[Ashutosh Bapat]

On Tue, Mar 28, 2017 at 10:24 PM, Robert Haas  wrote:
> On Mon, Mar 27, 2017 at 8:36 AM, Ashutosh Bapat
>  wrote:
>> I have gone through the patch, and it looks good to me. Here's the set
>> of patches with this patch included. Fixed the testcase failures.
>> Rebased the patchset on de4da168d57de812bb30d359394b7913635d21a9.
>
> This version of 0001 looks much better to me, but I still have some concerns.
>
> I think we should also introduce IS_UPPER_REL() at the same time, for
> symmetry and because partitionwise aggregate will need it, and use it
> in place of direct tests against RELOPT_UPPER_REL.

Ok. Done. I introduced IS_JOIN_REL and IS_OTHER_REL only to simplify
the tests for child-joins. But now we have grown this patch with
IS_SIMPLE_REL() and IS_UPPER_REL(). That has introduced changes
unrelated to partition-wise join. But I am happy with the way the code
looks now with all IS_*_REL() macros. If we delay this commit, some
more usages of bare RELOPT_* would creep in the code. To avoid that,
we may want to commit these changes in v10.

>
> I think it would make sense to change the test in deparseFromExpr() to
> check for IS_JOIN_REL() || IS_SIMPLE_REL().  There's no obvious reason
> why that shouldn't be OK, and it would remove the last direct test
> against RELOPT_JOINREL in the tree, and it will probably need to be
> changed for partitionwise aggregate anyway.

Done. However, we need another assertion to make sure than an "other"
upper rel has an "other" rel as scanrel. That can be added when
partition-wise aggregate, which would introduce "other" upper rels, is
implemented.

>
> Could set_append_rel_size Assert(IS_SIMPLE_REL(rel))?  I notice that
> you did this in some other places such as
> generate_implied_equalities_for_column(), and I like that.  If for
> some reason that's not going to work, then it's doubtful whether
> Assert(childrel->reloptkind == RELOPT_OTHER_MEMBER_REL) is going to
> survive either.

Done. Also modified prologue of that function to explicitly say simple
"append relation" since we can have join "append relations" and upper
"append relations" with partition-wise operations.

>
> Similarly, I think relation_excluded_by_constraints() would also
> benefit from Assert(IS_SIMPLE_REL(rel)).

Done.

>
> Why not set top_parent_relids earlier, when actually creating the
> RelOptInfo?  I think you could just change build_simple_rel() so that
> instead of passing RelOptKind reloptkind, you instead pass RelOptInfo
> *parent.  I think postponing that work until set_append_rel_size()
> just introduces possible bugs resulting from it not being set early
> enough.

Done.

>
> Apart from the above, I think 0001 is in good shape.
>
> Regarding 0002, I think the parts that involve factoring out
> find_param_path_info() are uncontroversial.  Regarding the changes to
> adjust_appendrel_attrs(), my main question is whether we wouldn't be
> better off using an array representation rather than a List
> representation.  In other words, this function could take PlannerInfo
> *root, Node *node, int nappinfos, AppendRelInfo **appinfos.  Existing
> callers doing adjust_appendrel_attrs(root, whatever, appinfo) could
> just do adjust_appendrel_attrs(root, whatever, 1, ), not
> needing to allocate.  To make this work, adjust_child_relids() and
> find_appinfos_by_relids() would need to be adjusted to use a similar
> argument-passing convention.  I suspect this makes iterating over the
> AppendRelInfos mildly faster, too, apart from the memory savings.

Done.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: Batch/pipelining support for libpq

[Vaishnavi Prabakaran]

On Thu, Mar 30, 2017 at 12:08 PM, Michael Paquier  wrote:

> On Wed, Mar 29, 2017 at 12:40 PM, Vaishnavi Prabakaran
>  wrote:
> > Michael Paquier wrote:
> >>Could you as well update src/tools/msvc/vcregress.pl, aka the routine
> >>modulescheck so as this new test is skipped. I am sure that nobody
> >>will scream if this test is not run on Windows, but the buildfarm will
> >>complain if the patch is let in its current state.
> >
> > Thanks for the finding. Hmm, modulescheck will not pick up test_libpq as
> > "subdircheck" will fail for this module(because it does not have
> > "sql"/"expected" folders in it) and hence it will be skipped.
> > But, Modified "Mkvcbuild.pm" to exclude "test_libpq" module, as this test
> > anyways will not be run in Windows and also because it uses linux
> specific
> > APIs(gettimeofday , timersub).
>
> src/port/gettimeofday.c extends things on Windows, and we could
> consider to just drop the timing portion for simplicity (except
> Windows I am not seeing any platform missing timersub). But that's
> just a point of detail. Or we could just drop those tests from the
> final patch, and re-implement them after having some psql-level
> subcommands. That would far better for portability.
>

Yes agree, having tests with psql meta commands will be more easy to
understand also.  And, that is one of the reason I separated the patch into
two - code and test . So, yes, we can drop the test patch for now.


>
> > There are 2 cases tested here:
> >
> > 1. Example for the case - Using COPY command in batch mode will trigger
> > failure. (Specified in documentation)
> > Failure can be seen only when processing the copy command's results. That
> > is, after dispatching COPY command, server goes into COPY state , but the
> > client dispatched another query in batch mode.
> >
> > Below error messages belongs to this case :
> > Error status and message got from server due to COPY command failure are
> :
> > PGRES_FATAL_ERROR ERROR:  unexpected message type 0x50 during COPY from
> > stdin
> > CONTEXT:  COPY batch_demo, line 1
> >
> > message type 0x5a arrived from server while idle
>
> Such messages are really confusing to the user as they refer to the
> protocol going out of sync. I would think that something like "cannot
> process COPY results during a batch processing" would be cleaner.
> Isn't some more error handling necessary in GetCopyStart()?


Hmm, With batch mode, after sending COPY command to server(and server
started processing the query and goes into COPY state) , client does not
immediately read the result , but it keeps sending other queries to the
server. By this time, server already encountered the error
scenario(Receiving different message during COPY state) and sent error
messages. So, when client starts reading the result, already error messages
sent by server are present in socket.  I think trying to handle during
result processing is more like masking the server's error message with new
error message and I think it is not appropriate.



>
>
> Attached the latest code and test patches.
>
> For me the test is still broken:
> ok 1 - testlibpqbatch disallowed_in_batch
> ok 2 - testlibpqbatch simple_batch
> ok 3 - testlibpqbatch multi_batch
> ok 4 - testlibpqbatch batch_abort
> ok 5 - testlibpqbatch timings
> not ok 6 - testlibpqbatch copyfailure
>
> Still broken here. I can see that this patch is having enough
> safeguards in PQbatchBegin() and PQsendQueryStart(), but this issue is
> really pointing out at something...
>
>
I will check this one with fresh setup again and I guess that this should
not block the code patch.

Thanks & Regards,
Vaishnavi
Fujitsu Australia.

Re: [HACKERS] [BUGS] Bug in Physical Replication Slots (at least 9.5)?

[Venkata B Nagothi]

On Thu, Mar 30, 2017 at 3:51 PM, Kyotaro HORIGUCHI <
horiguchi.kyot...@lab.ntt.co.jp> wrote:

> At Thu, 30 Mar 2017 11:12:56 +1100, Venkata B Nagothi 
> wrote in  gmail.com>
> > On Thu, Mar 30, 2017 at 10:55 AM, Michael Paquier <
> michael.paqu...@gmail.com
> > > wrote:
> >
> > > On Thu, Mar 30, 2017 at 8:49 AM, Venkata B Nagothi 
> > > wrote:
> > > > On Tue, Mar 28, 2017 at 5:51 PM, Kyotaro HORIGUCHI
> > > >  wrote:
> > > > I tried applying this patch to latest master, it is not getting
> applied
> > > >
> > > > [dba@buildhost postgresql]$ git apply
> > > > /data/postgresql-patches/9.5-ReplicationSlots-Bug-Patch/
> > > 0001-Fix-a-bug-of-physical-replication-slot_a6f22e8.patch
> > > > /data/postgresql-patches/9.5-ReplicationSlots-Bug-Patch/
> > > 0001-Fix-a-bug-of-physical-replication-slot_a6f22e8.patch:28:
> > > > trailing whitespace.
> > > > /*
> > > 0001-Fix-a-bug-of-physical-replication-slot_a6f22e8.patch:29:
> > > > trailing whitespace.
> > > >  * This variable corresponds to restart_lsn in pg_replication_slots
> for a
> ...
> > > git apply and git am can be very picky sometimes, so you may want to
> > > fallback to patch -p1 if things don't work. In this case it does.
> > >
> >
> > patch -p1 seems to be working. Thanks !
>
> That's quite strange. The patch I sent doesn't cantain trailing
> spaces at all. The cited lines doesn't seem to contain them. It
> applied cleanly with "git am" for me.
>
> The file restored from the attachment of received mail also don't.
>
> The original files contains the following,
>
> 0002440  66  6f  72  20  61  0a  2b  20  2a  20  70  68  79  73  69  63
>   f   o   r   a  \n   +   *   p   h   y   s   i   c
>
> The corresponding part of the file restored from mail on Windows
> is the following,
> 0002460  63  61  74  69  6f  6e  5f  73  6c  6f  74  73  20  66  6f  72
>   c   a   t   i   o   n   _   s   l   o   t   s   f   o   r
> 0002500  20  61  0d  0a  2b  20  2a  20  70  68  79  73  69  63  61  6c
>   a  \r  \n   +   *   p   h   y   s   i   c   a   l
>
> Both doesn't contain a space at the end of a line. How did you
> retrieve the patch from the mail?
>

Yes, downloaded from the email on Windows and copied across to Linux and
did "git apply".

Regards,

Venkata B N
Database Consultant

Re: [HACKERS] [BUGS] Bug in Physical Replication Slots (at least 9.5)?

[Kyotaro HORIGUCHI]

At Thu, 30 Mar 2017 11:12:56 +1100, Venkata B Nagothi  wrote 
in 
> On Thu, Mar 30, 2017 at 10:55 AM, Michael Paquier  > wrote:
> 
> > On Thu, Mar 30, 2017 at 8:49 AM, Venkata B Nagothi 
> > wrote:
> > > On Tue, Mar 28, 2017 at 5:51 PM, Kyotaro HORIGUCHI
> > >  wrote:
> > > I tried applying this patch to latest master, it is not getting applied
> > >
> > > [dba@buildhost postgresql]$ git apply
> > > /data/postgresql-patches/9.5-ReplicationSlots-Bug-Patch/
> > 0001-Fix-a-bug-of-physical-replication-slot_a6f22e8.patch
> > > /data/postgresql-patches/9.5-ReplicationSlots-Bug-Patch/
> > 0001-Fix-a-bug-of-physical-replication-slot_a6f22e8.patch:28:
> > > trailing whitespace.
> > > /*
> > 0001-Fix-a-bug-of-physical-replication-slot_a6f22e8.patch:29:
> > > trailing whitespace.
> > >  * This variable corresponds to restart_lsn in pg_replication_slots for a
...
> > git apply and git am can be very picky sometimes, so you may want to
> > fallback to patch -p1 if things don't work. In this case it does.
> >
> 
> patch -p1 seems to be working. Thanks !

That's quite strange. The patch I sent doesn't cantain trailing
spaces at all. The cited lines doesn't seem to contain them. It
applied cleanly with "git am" for me.

The file restored from the attachment of received mail also don't.

The original files contains the following,

0002440  66  6f  72  20  61  0a  2b  20  2a  20  70  68  79  73  69  63
  f   o   r   a  \n   +   *   p   h   y   s   i   c

The corresponding part of the file restored from mail on Windows
is the following,
0002460  63  61  74  69  6f  6e  5f  73  6c  6f  74  73  20  66  6f  72
  c   a   t   i   o   n   _   s   l   o   t   s   f   o   r
0002500  20  61  0d  0a  2b  20  2a  20  70  68  79  73  69  63  61  6c
  a  \r  \n   +   *   p   h   y   s   i   c   a   l

Both doesn't contain a space at the end of a line. How did you
retrieve the patch from the mail?

regarsd,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Allow interrupts on waiting standby

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
> > By the way, doesn't this wait event belong to IPC wait event type, because
> the process is waiting for other conflicting processes to terminate the
> conflict conditions?  Did you choose Timeout type because
> max_standby_{archive | streaming}_delay relates to this wait?  I'm not
> confident which is appropriate, but I'm afraid users can associate this
> wait with a timeout.
> 
> Actually I think that this event belongs to the timeout category, because
> we wait until the timeout has finished, the latch being here to be sure
> that the process is more responsive in case of a postmaster death.

OK.  I confirmed the doc is fixed.

> > (2) standby.c
> > Do we need to specify WL_LATCH_SET?  Who can set the latch?  Do the
> backends who ended the conflict set the latch?
> 
> This makes the process able to react on SIGHUP. That's useful for
> responsiveness.

Oh, I see.  But how does the startup process respond quickly?  It seems that 
you need to call HandleStartupProcInterrupts() instead of 
CHECK_FOR_INTERRUPTS().  But I'm not sure whether HandleStartupProcInterrupts() 
can be called here.



> > (3) standby.c
> > +   if (rc & WL_LATCH_SET)
> > +   ResetLatch(MyLatch);
> > +
> > +   /* emergency bailout if postmaster has died */
> > +   if (rc & WL_POSTMASTER_DEATH)
> > +   proc_exit(1);
> >
> > I thought the child processes have to terminate as soon as postmaster
> vanishes.  So, it would be better for the order of the two if statements
> above to be reversed.  proc_exit() could be exit(), as some children do
> in postmaster/*.c.
> 
> OK, reversed this order.

I think exit() instead of proc_exit() better represents what the code wants to 
do -- terminate the process ASAP without cleaning up.  Many other background 
children do so.


Regards
Takayuki Tsunakawa


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Guidelines for GSoC student proposals / Eliminate O(N^2) scaling from rw-conflict tracking in serializable transactions

[Mengxing Liu]

Thanks, I've updated the proposal. Just one issue: 
I agree that we can make skip list a general data structure.  But can we use 
the fixed-level skip list as a Plan B? Or a quick attempt before the general 
data structure ? 
Because I am not familiar with shared memory structure and tricks used in it, 
and I cannot estimate how much time it would take. 


> -Original Messages-
> From: "Kevin Grittner" 
> Sent Time: 2017-03-28 00:16:11 (Tuesday)
> To: "Mengxing Liu" 
> Cc: "pgsql-hackers@postgresql.org" 
> Subject: Re: [HACKERS] Guidelines for GSoC student proposals / Eliminate 
> O(N^2) scaling from rw-conflict tracking in serializable transactions
> 
> On Sat, Mar 25, 2017 at 9:24 PM, Mengxing Liu
>  wrote:
> 
> > I've updated the proposal according to your comments.
> > But I am still wondering that can you review it for a double-check
> > to make sure I've made everything clear?
> 
> Additional comments added.
> 
> I'm afraid a few new issues came to mind reading it again.  (Nothing
> serious; just some points that could benefit from a little
> elaboration.)
> 
> --
> Kevin Grittner
> 
> 
> -- 
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers


--
Mengxing Liu










-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Tom Lane]

Fabien COELHO  writes:
>> New Patch v29: Now with less coverage!

> Patch applies cleanly. Make check ok. Feature still works!

I've been hacking on this for about two full days now, and have gotten
it to a point where I think it's committable.  Aside from cosmetic
changes, I've made it behave reasonably for cases where \if is used
on portions of a query, for instance

SELECT
\if :something
var1
\else
var2
\endif
FROM table;

which as I mentioned a long time ago is something that people will
certainly expect to work.  I also cleaned up a lot of corner-case
discrepancies between how much text is consumed in active-branch and
inactive-branch cases (OT_FILEPIPE is a particularly nasty case in that
regard :-()

I plan to read this over again tomorrow and then push it, if there are
not objections/corrections.

regards, tom lane

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 2a9c412..b51b11b 100644
*** a/doc/src/sgml/ref/psql-ref.sgml
--- b/doc/src/sgml/ref/psql-ref.sgml
*** hello 10
*** 2064,2069 
--- 2064,2158 
  
  

+ \if expression
+ \elif expression
+ \else
+ \endif
+ 
+ 
+ This group of commands implements nestable conditional blocks.
+ A conditional block must begin with an \if and end
+ with an \endif.  In between there may be any number
+ of \elif clauses, which may optionally be followed
+ by a single \else clause.  Ordinary queries and
+ other types of backslash commands may (and usually do) appear between
+ the commands forming a conditional block.
+ 
+ 
+ The \if and \elif commands read
+ their argument(s) and evaluate them as a boolean expression.  If the
+ expression yields true then processing continues
+ normally; otherwise, lines are skipped until a
+ matching \elif, \else,
+ or \endif is reached.  Once
+ an \if or \elif test has
+ succeeded, the arguments of later \elif commands in
+ the same block are not evaluated but are treated as false.  Lines
+ following an \else are processed only if no earlier
+ matching \if or \elif succeeded.
+ 
+ 
+ The expression argument
+ of an \if or \elif command
+ is subject to variable interpolation and backquote expansion, just
+ like any other backslash command argument.  After that it is evaluated
+ like the value of an on/off option variable.  So a valid value
+ is any unambiguous case-insensitive match for one of:
+ true, false, 1,
+ 0, on, off,
+ yes, no.  For example,
+ t, T, and tR
+ will all be considered to be true.
+ 
+ 
+ Expressions that do not properly evaluate to true or false will
+ generate a warning and be treated as false.
+ 
+ 
+ Lines being skipped are parsed normally to identify queries and
+ backslash commands, but queries are not sent to the server, and
+ backslash commands other than conditionals
+ (\if, \elif,
+ \else, \endif) are
+ ignored.  Conditional commands are checked only for valid nesting.
+ Variable references in skipped lines are not expanded, and backquote
+ expansion is not performed either.
+ 
+ 
+ All the backslash commands of a given conditional block must appear in
+ the same source file. If EOF is reached on the main input file or an
+ \include-ed file before all local
+ \if-blocks have been closed,
+ then psql will raise an error.
+ 
+ 
+  Here is an example:
+ 
+ 
+ -- check for the existence of two separate records in the database and store
+ -- the results in separate psql variables
+ SELECT
+ EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer,
+ EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee
+ \gset
+ \if :is_customer
+ SELECT * FROM customer WHERE customer_id = 123;
+ \elif :is_employee
+ \echo 'is not a customer but is an employee'
+ SELECT * FROM employee WHERE employee_id = 456;
+ \else
+ \if yes
+ \echo 'not a customer or employee'
+ \else
+ \echo 'this will never print'
+ \endif
+ \endif
+ 
+ 
+   
+ 
+ 
+   
  \l[+] or \list[+] [ pattern ]
  
  
*** testdb= INSERT INTO my_ta
*** 3715,3721 
  
  
  In prompt 1 normally =,
! but ^ if in single-line mode,
  or ! if the session is disconnected from the
  database (which can happen if \connect fails).
  In prompt 2 %R is replaced by a character that
--- 3804,3811 
  
  
  In prompt 1 normally =,
! but @ if 

Re: [HACKERS] sequence data type

[Vitaly Burovoy]

On 3/29/17, Michael Paquier  wrote:
> On Thu, Mar 30, 2017 at 11:18 AM, Vitaly Burovoy
>  wrote:
>> I think min_value and max_value should not be set to "1" or "-1" but
>> to real min/max of the type by default.
>
> This is the default behavior for ages, since e8647c45 to be exact. So
> you would change 20 years of history?
> --
> Michael
>

Unfortunately yes, because the current patch has appeared because of
another patch with the "IDENTITY COLUMN" feature.
Since sequences used for such columns are completely hidden (they do
not appear in DEFAULTs like "serial" do) and a new option (sequence
data type) is supported with its altering (which was absent
previously), new errors appear because of that.

Be honest I did not checked the "countdown" case at the current
release, but the most important part is the second one because it is a
direct analogue of what happens (in the parallel thread) on "ALTER
TABLE tbl ALTER col TYPE new_type" where "col" is an identity column.

Since the commit 2ea5b06c7a7056dca0af1610aadebe608fbcca08 declares
"The data type determines the default minimum and maximum values of
the sequence." is it a wrong way to keep historical minimum as "1" by
default: it is not a minimum of any of supported type.

I don't think something will break if min_value is set lesser than
before, but it will decrease astonishment of users in cases I wrote in
the previous email.

-- 
Best regards,
Vitaly Burovoy


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PG_GETARG_GISTENTRY?

[Tom Lane]

Andres Freund  writes:
> we have a good number of '(GISTENTRY *) PG_GETARG_POINTER(n)' in our
> code - looks a bit better & shorter to have PG_GETARG_GISTENTRY(n).

Should be PG_GETARG_GISTENTRY_P to match existing conventions,
otherwise +1

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Logical decoding on standby

[Craig Ringer]

On 29 March 2017 at 23:13, Simon Riggs  wrote:
> On 29 March 2017 at 10:17, Craig Ringer  wrote:
>> On 29 March 2017 at 16:44, Craig Ringer  wrote:
>>
>>> * Split oldestCatalogXmin tracking into separate patch
>>
>> Regarding this, Simon raised concerns about xlog volume here.
>>
>> It's pretty negligible.
>>
>> We only write a new record when a vacuum runs after catalog_xmin
>> advances on the slot with the currently-lowest catalog_xmin (or, if
>> vacuum doesn't run reasonably soon, when the bgworker next looks).
>
> I'd prefer to slow things down a little, not be so eager.
>
> If we hold back update of the catalog_xmin until when we run
> GetRunningTransactionData() we wouldn't need to produce any WAL
> records at all AND we wouldn't need to have VACUUM do
> UpdateOldestCatalogXmin(). Bgwriter wouldn't need to perform an extra
> task.
>
> That would also make this patch about half the length it is.
>
> Let me know what you think.

Good idea.

We can always add a heuristic later to make xl_running_xacts get
emitted more often at high transaction rates if it's necessary.

Patch coming soon.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] sequence data type

[Michael Paquier]

On Thu, Mar 30, 2017 at 11:18 AM, Vitaly Burovoy
 wrote:
> I think min_value and max_value should not be set to "1" or "-1" but
> to real min/max of the type by default.

This is the default behavior for ages, since e8647c45 to be exact. So
you would change 20 years of history?
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] TPC-H Q20 from 1 hour to 19 hours!

[Rafia Sabih]

On Thu, Mar 30, 2017 at 12:30 AM, Robert Haas  wrote:

> I don't think the problem originates at the Merge Join, though,
> because the commit says that at is fixing semi and anti-join estimates
> - this is a plain inner join, so in theory it shouldn't change.
> However, it's a bit hard for me to piece through these plans, the
> formatting kind of got messed up - things are wrapped.  Could you
> possibly attach the plans as attachments?
>
Sure, please find attached file for the plans before and after commit.

-- 
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/
The plan that completes in 19 hours after commit:
commit 7fa93eec4e0c9c3e801e3c51aa4bae3a38aaa218
Author: Tom Lane 
Date:   Sat Dec 17 15:28:54 2016 -0500
 Fix FK-based join selectivity estimation for semi/antijoins.

QUERY PLAN
-
 Limit  (cost=60187550.59..60187550.59 rows=1 width=52) (actual 
time=71064602.963..71064602.964 rows=1 loops=1)
   ->  Sort  (cost=60187550.59..60187550.59 rows=3 width=52) (actual 
time=71064602.961..71064602.961 rows=1 loops=1)
 Sort Key: supplier.s_name
 Sort Method: top-N heapsort  Memory: 25kB
 ->  Nested Loop Semi Join  (cost=52960550.15..60187550.57 rows=3 
width=52) (actual time=2163639.699..71063153.953 rows=52263 loops=1)
   Join Filter: (supplier.s_suppkey = lineitem.l_suppkey)
   Rows Removed by Join Filter: 155706242106
   ->  Nested Loop  (cost=963.43..10081.54 rows=12 width=56) 
(actual time=178.589..6282.852 rows=120358 loops=1)
 ->  Seq Scan on nation  (cost=0.00..0.41 rows=1 width=4) 
(actual time=0.018..0.042 rows=1 loops=1)
   Filter: (n_name = 'EGYPT'::bpchar)
   Rows Removed by Filter: 24
 ->  Bitmap Heap Scan on supplier  (cost=963.43..8881.13 
rows=12 width=64) (actual time=178.563..6143.786 rows=120358 loops=1)
   Recheck Cond: (s_nationkey = nation.n_nationkey)
   Heap Blocks: exact=57317
   ->  Bitmap Index Scan on idx_supplier_nation_key  
(cost=0.00..933.43 rows=12 width=0) (actual time=133.218..133.218 
rows=120358 loops=1)
 Index Cond: (s_nationkey = nation.n_nationkey)
   ->  Materialize  (cost=52959586.72..60024469.24 rows=85 
width=16) (actual time=12.679..175.456 rows=1293693 loops=120358)
 ->  Merge Join  (cost=52959586.72..60024468.82 rows=85 
width=16) (actual time=1525322.753..2419045.641 rows=1696742 loops=1)
   Merge Cond: ((lineitem.l_partkey = 
partsupp.ps_partkey) AND (lineitem.l_suppkey = partsupp.ps_suppkey))
   Join Filter: ((partsupp.ps_availqty)::numeric > 
((0.5 * sum(lineitem.l_quantity
   Rows Removed by Join Filter: 3771
   ->  GroupAggregate  (cost=48448912.90..53325163.98 
rows=144696357 width=48) (actual time=1342085.116..2178225.340 rows=156665300 
loops=1)
 Group Key: lineitem.l_partkey, 
lineitem.l_suppkey
 ->  Sort  (cost=48448912.90..49125364.33 
rows=270580572 width=21) (actual time=1342085.072..1495863.254 rows=273057944 
loops=1)
   Sort Key: lineitem.l_partkey, 
lineitem.l_suppkey
   Sort Method: external merge  Disk: 
8282600kB
   ->  Bitmap Heap Scan on lineitem  
(cost=2847641.84..10552097.42 rows=270580572 width=21) (actual 
time=241170.637..650122.225 rows=273058377 loops=1)
 Recheck Cond: ((l_shipdate >= 
'1994-01-01'::date) AND (l_shipdate < '1995-01-01 00:00:00'::timestamp without 
time zone))
 Heap Blocks: exact=3333
 ->  Bitmap Index Scan on 
idx_l_shipdate  (cost=0.00..2779996.70 rows=270580572 width=0) (actual 
time=190321.155..190321.155 rows=273058377 loops=1)
   Index Cond: ((l_shipdate >= 
'1994-01-01'::date) AND (l_shipdate < '1995-01-01 00:00:00'::timestamp without 
time zone))
   ->  Sort  (cost=4510673.81..4516734.42 rows=2424244 
width=24) (actual time=183237.183..184039.914 rows=2602656 loops=1)
 Sort Key: partsupp.ps_partkey, 
partsupp.ps_suppkey
 Sort Method: quicksort  Memory: 301637kB
 ->  Hash Join  (cost=379620.36..4253593.60 
rows=2424244 width=24) (actual time=8576.343..179703.563 rows=2602656 loops=1)
   

Re: [HACKERS] [WIP] RE: DECLARE STATEMENT setting up a connection in ECPG

[Ideriha, Takeshi]

Thank you very much for thorough review and sorry for late replay.
Attched is 002_declareStmt_ecpglib_v7.patch and I haven't revised doc patch yet.

>002_declareStmt_ecpglib_v5.patch:

>+ struct connection *f = NULL;
>+
>  ecpg_init_sqlca(sqlca);
>  for (con = all_connections; con;)
>  {
>- struct connection *f = con;
>+ f = con;

>What is the need of moving the structure declartion
>to outside, i didn't find any usage of it later.

Fixed.

>+ con = ecpg_get_connection(connection_name);
>+ if (!con)
>+ {
>+ return;
>+ }
>
>No need of {}.

Fixed.

>+ if (find_cursor(cursor_name, con))
>+ {
>+ /*
>+ * Should never goto here, because ECPG has checked the duplication of
>+ * the cursor in pre-compile stage.
>+ */
>+ return;
>+ }
>
>Do we really need this check? If it is for additional check, How about
>checking
>it with an Assert? (check for similar instances)

I remove the above check because we check the duplication when parsing 
ECPGCursorStmt token at ecpg.trailer and in the existing master code we also 
check the cursor name duplication only when pre-compilineg pgc code.

Regarding similar codes, I added ecpg_raise() assertion.

>+ if (!ecpg_init(con, real_connection_name, line))
>+ return false;
>
>What is the need of ecpg_init call? why the same is not done in other
>functions.
>Better if you provide some comments about the need of the function call.

Removed ecpg_init() and added checking if con exists or not.

>-#endif   /* _ECPG_LIB_EXTERN_H */
>+#endif   /* _ECPG_LIB_EXTERN_H */
>
>Not related change.

Fixed.

>+ * Find the declared name referred by the cursor,
>+ * then return the connection name used by the declared name.
>
>How about rewriting the above statement as follows? This is because
>we are not finding the declare name, as we are looping through all
>the declare statements for this cursor.
>
>"Find the connection name by referring the declared statements
>cursors by using the provided cursor name"

Fixed.

>+ struct declared_statement *cur = NULL;
>+ struct declared_statement *prev = NULL;
>+ struct declared_statement *next = NULL;
>
>The above logic can be written without "next" pointer.
>That way it should be simpler.

Fixed.

>-#endif   /* _ECPGTYPE_H */
>+#endif /* _ECPGTYPE_H */
>
>Not related change.

Fixed.


>+ if(connection_name == NULL)
>+ {
>+ /*
>+ * Going to here means not using AT clause in the DECLARE STATEMENT
>+ * We don't allocate a node to store the declared name because the
>+ * DECLARE STATEMENT without using AT clause will be ignored.
>+ */
>+ return true;
>+ }
>
>I am not sure that just ignore the declare statement may be wrong.
>I feel whether such case is possible? Does the preprocessor allows it?

As you pointed out, the above thing should be discussed.
The current implementation is as follows:

ECPG pre-processor allows the DECLARE STATEMENT without using AT clause.
And the following statement after DECLARE STATEMENT such as PREPARE, EXECUTE 
are 
executed as usual on the current connection.

But there's a limitation here.
 (This limitation should be disccused earlier and be specified in the doc...
  but I didn't realize this clearly by myself, sorry)

When using DECLARE STATEMENT without AT clause
and using OPEN statement with AT clause, it doesn't work.

There's an example where you cannot fetch rows from db:
EXEC SQL CONNECT TO db AS con;

EXEC SQL DECLARE stmt STATEMENT;
EXEC SQL AT con PREPARE stmt FROM :selectString;
EXEC SQL AT con DECLARE cur CURSOR FOR stmt;
EXEC SQL AT con OPEN cur;
 ...

This limitation looks troublesome for users,
so maybe I need to fix this implementation. 

regards,
Ideriha Takeshi


002_declareStmt_ecpglib_v7.patch
Description: 002_declareStmt_ecpglib_v7.patch

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] TPC-H Q20 from 1 hour to 19 hours!

[Robert Haas]

On Wed, Mar 29, 2017 at 8:00 PM, Tomas Vondra
 wrote:
> What is however strange is that changing max_parallel_workers_per_gather
> affects row estimates *above* the Gather node. That seems a bit, um,
> suspicious, no? See the parallel-estimates.log.

Thanks for looking at this!  Comparing the parallel plan vs. the
non-parallel plan:

part: parallel rows (after Gather) 20202, non-parallel rows 20202
partsupp: parallel rows 18, non-parallel rows 18
part-partsupp join: parallel rows 88988, non-parallel rows 355951
lineitem: parallel rows 59986112, non-parallel rows 59986112
lineitem after aggregation: parallel rows 5998611, non-parallel rows 5998611
final join: parallel rows 131, non-parallel rows 524

I agree with you that that looks mighty suspicious.  Both the
part-partsupp join and the final join have exactly 4x as many
estimated rows in the non-parallel plan as in the parallel plan, and
it just so happens that the parallel divisor here will be 4.

Hmm... it looks like the parallel_workers value from the Gather node
is being erroneously propagated up to the higher levels of the plan
tree.   Wow.   Somehow, Gather Merge managed to get the logic correct
here, but Gather is totally wrong.  Argh.   Attached is a draft patch,
which I haven't really tested beyond checking that it passes 'make
check'.

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


fix-gather-worker-propagation.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] sequence data type

[Vitaly Burovoy]

On 3/29/17, Peter Eisentraut  wrote:
> Over at
> 
> is is being discussed that maybe the behavior when altering the sequence
> type isn't so great, because it currently doesn't update the min/max
> values of the sequence at all.  So here is a patch to update the min/max
> values when the old min/max values were the min/max values of the data
> type.
>
> --
> Peter Eisentraut  http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

It seems almost good for me except a single thing (I'm sorry, I missed
the previous discussion).
Why is min_value set to 1 (or -1 for negative INCREMENTs) by default
for all sequence types?
With the committed patch it leads to the extra "MINVALUE" option
besides the "START" one; and it is not the worst thing.

It leads to strange error for countdown sequences:
postgres=# CREATE SEQUENCE abc AS smallint MINVALUE 0 START 2 INCREMENT -1;
ERROR:  MINVALUE (0) must be less than MAXVALUE (-1)

postgres=# CREATE SEQUENCE abc AS smallint MINVALUE 0 START 2
INCREMENT -1 NO MAXVALUE; -- does not help
ERROR:  MINVALUE (0) must be less than MAXVALUE (-1)


With the proposed patch users can impact with the next error:

postgres=# CREATE TABLE tbl(i smallserial);
CREATE TABLE
postgres=# SELECT * FROM pg_sequences;
 schemaname | sequencename | sequenceowner | data_type | start_value |
min_value | max_value | increment_by | cycle | cache_size | last_value
+--+---+---+-+---+---+--+---++
 public | tbl_i_seq| burovoy_va| smallint  |   1 |
1 | 32767 |1 | f |  1 |
(1 row)

postgres=# -- min_value for smallint is "1"? Ok, I want to use the whole range:
postgres=# ALTER SEQUENCE tbl_i_seq MINVALUE -32768 START -32768 RESTART -32768;
ALTER SEQUENCE
postgres=# -- after a while I realized the range is not enough. Try to
enlarge it:
postgres=# ALTER SEQUENCE tbl_i_seq AS integer;
ERROR:  START value (-32768) cannot be less than MINVALUE (1)

It is not an expected behavior.

I think min_value and max_value should not be set to "1" or "-1" but
to real min/max of the type by default.

I recommend to add to the docs explicit phrase that START value is not
changed even if it matches the bound of the original type.

Also it is good to have regressions like:
CREATE SEQUENCE sequence_test10 AS smallint  MINVALUE -1000 MAXVALUE 1000;
ALTER SEQUENCE sequence_test10 AS int NO MINVALUE NO MAXVALUE INCREMENT 1;
ALTER SEQUENCE sequence_test10 AS bigint NO MINVALUE NO MAXVALUE INCREMENT -1;

CREATE SEQUENCE sequence_test11 AS smallint  MINVALUE -32768 MAXVALUE 32767;
ALTER SEQUENCE sequence_test11 AS int NO MINVALUE NO MAXVALUE INCREMENT 1;
ALTER SEQUENCE sequence_test11 AS int NO MINVALUE NO MAXVALUE INCREMENT -1;

-- 
Best regards,
Vitaly Burovoy


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_stat_wal_write statistics view

[Haribabu Kommi]

On Wed, Mar 29, 2017 at 5:10 AM, Fujii Masao  wrote:

> On Tue, Mar 28, 2017 at 1:40 PM, Haribabu Kommi
>  wrote:
> >
> >
> > Added stats collection for walsender, statrup and autovacuum processes.
> > The background workers that call pgstat_report_stat() function will
> > automatically
> > included.
>
> So you added pgstat_send_walwrites() into several places. But WAL activity
> by the background workers which don't call pgstat_report_stat() is still
> not
> considered in pg_stat_walwrites view. This design looks fragile to me.
> I think that we should capture all the WAL activities (even by such
> background
> workers) in XLogWrite(), instead.
>
> For example, we can add the "shared" counters (protected by WALWriteLock)
> into XLogCtl and make XLogWrite() update those counters if the process is
> NOT walwriter. OTOH, if it's walwriter, XLogWrite() updates its local
> counters.
> Then walwriter periodically packs those counters in the message and sends
> it to the stats collector.
>

Updated patch to use shared counter instead of adding pg_stat_ calls to send
the statistics from each background process/worker.

All the processes updates the shared counters, the walwriter process fill
the
local structure with the shared counters and send it to the stats collector.


> >
> > update patch attached.
>
> Thanks for updating the patch!
>
> +  writes
> +  bigint
> +  
> +  Number of WAL writes that are carried out by background
> processes and workers.
> +  
> 
> +  write_blocks
> +  bigint
> +  Number of WAL pages written to the disk by the
> background processes/workers
> + 
>
> Could you tell me why both numbers of WAL writes and pages written need to
> be reported? How can we tune the system by using those information?
>

With the column of write_blocks, it is useful to find out how many number
of blocks
that gets written from backend along with the number of writes. I feel
1. If we only get the count of number of writes and the write amount is
very less,
so it may not need any tuning.
2. If the amount of write is good even for less number of writes, then it
needs
some tuning.

Updated patch attached.

Regards,
Hari Babu
Fujitsu Australia


pg_stat_walwrites_view_5.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] [PATCH] Add two-arg for of current_setting(NAME, FALLBACK)

[David Christensen]

The two-arg form of the current_setting() function will allow a
fallback value to be returned instead of throwing an error when an
unknown GUC is provided.  This would come in most useful when using
custom GUCs; e.g.:

  -- errors out if the 'foo.bar' setting is unset
  SELECT current_setting('foo.bar');

  -- returns current setting of foo.bar, or 'default' if not set
  SELECT current_setting('foo.bar', 'default')

This would save you having to wrap the use of the function in an
exception block just to catch and utilize a default setting value
within a function.
---
 src/backend/utils/misc/guc.c  | 50 ---
 src/include/catalog/pg_proc.h |  2 ++
 src/include/utils/builtins.h  |  1 +
 src/include/utils/guc.h   |  1 +
 src/test/regress/expected/guc.out | 19 +++
 src/test/regress/sql/guc.sql  | 12 ++
 6 files changed, 82 insertions(+), 3 deletions(-)

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 26275bd..002a926 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -7703,13 +7703,32 @@ ShowAllGUCConfig(DestReceiver *dest)
 char *
 GetConfigOptionByName(const char *name, const char **varname)
 {
+   return GetConfigOptionByNameFallback(name, NULL, varname);
+}
+
+/*
+ * Return GUC variable value by name; optionally return canonical form of
+ * name.  If GUC is NULL then optionally return a fallback value instead of an
+ * error.  Return value is palloc'd.
+ */
+char *
+GetConfigOptionByNameFallback(const char *name, const char *default_value, 
const char **varname)
+{
struct config_generic *record;
 
record = find_option(name, false, ERROR);
if (record == NULL)
-   ereport(ERROR,
-   (errcode(ERRCODE_UNDEFINED_OBJECT),
-  errmsg("unrecognized configuration parameter 
\"%s\"", name)));
+   {
+   if (default_value) {
+   return pstrdup(default_value);
+   }
+   else
+   {
+   ereport(ERROR,
+   (errcode(ERRCODE_UNDEFINED_OBJECT),
+  errmsg("unrecognized configuration parameter 
\"%s\"", name)));
+   }
+   }
if ((record->flags & GUC_SUPERUSER_ONLY) && !superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
@@ -8008,6 +8027,31 @@ show_config_by_name(PG_FUNCTION_ARGS)
 }
 
 /*
+ * show_config_by_name_fallback - equiv to SHOW X command but implemented as
+ * a function.  If X does not exist, return a fallback datum instead of 
erroring
+ */
+Datum
+show_config_by_name_fallback(PG_FUNCTION_ARGS)
+{
+   char   *varname;
+   char   *varfallback;
+   char   *varval;
+   
+   /* Get the GUC variable name */
+   varname = TextDatumGetCString(PG_GETARG_DATUM(0));
+
+   /* Get the fallback value */
+   varfallback = TextDatumGetCString(PG_GETARG_DATUM(1));
+   
+   /* Get the value */
+   varval = GetConfigOptionByNameFallback(varname, varfallback, NULL);
+
+   /* Convert to text */
+   PG_RETURN_TEXT_P(cstring_to_text(varval));
+}
+
+
+/*
  * show_all_settings - equiv to SHOW ALL command but implemented as
  * a Table Function.
  */
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 6a757f3..71efed2 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -3025,6 +3025,8 @@ DESCR("convert bitstring to int8");
 
 DATA(insert OID = 2077 (  current_setting  PGNSP PGUID 12 1 0 0 0 f f f f 
t f s 1 0 25 "25" _null_ _null_ _null_ _null_ show_config_by_name _null_ _null_ 
_null_ ));
 DESCR("SHOW X as a function");
+DATA(insert OID = 3280 (  current_setting  PGNSP PGUID 12 1 0 0 0 f f f f 
t f s 2 0 25 "25 25" _null_ _null_ _null_ _null_ show_config_by_name_fallback 
_null_ _null_ _null_ ));
+DESCR("SHOW X as a function");
 DATA(insert OID = 2078 (  set_config   PGNSP PGUID 12 1 0 0 0 f f f f 
f f v 3 0 25 "25 25 16" _null_ _null_ _null_ _null_ set_config_by_name _null_ 
_null_ _null_ ));
 DESCR("SET X as a function");
 DATA(insert OID = 2084 (  pg_show_all_settings PGNSP PGUID 12 1 1000 0 0 f f f 
f t t s 0 0 2249 "" "{25,25,25,25,25,25,25,25,25,25,25,1009,25,25,25,23}" 
"{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}" 
"{name,setting,unit,category,short_desc,extra_desc,context,vartype,source,min_val,max_val,enumvals,boot_val,reset_val,sourcefile,sourceline}"
 _null_ show_all_settings _null_ _null_ _null_ ));
diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
index bc4517d..e3d9fbb 100644
--- a/src/include/utils/builtins.h
+++ b/src/include/utils/builtins.h
@@ -1088,6 +1088,7 @@ extern Datum quote_nullable(PG_FUNCTION_ARGS);
 
 /* guc.c */
 extern Datum show_config_by_name(PG_FUNCTION_ARGS);
+extern Datum 

Re: [HACKERS] sequence data type

[Michael Paquier]

On Thu, Mar 30, 2017 at 2:36 AM, Peter Eisentraut
 wrote:
> Over at
> 
> is is being discussed that maybe the behavior when altering the sequence
> type isn't so great, because it currently doesn't update the min/max
> values of the sequence at all.  So here is a patch to update the min/max
> values when the old min/max values were the min/max values of the data type.

Looks sane to me. The only comment I have would be to add a test to
trigger as well the code path of reset_min_value with MINVALUE.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Partitioning vs ON CONFLICT

[Shinoda, Noriyoshi]

Hello, 

I tried this feature using most recently snapshot. In case of added constraint 
PRIMARY KEY for partition table, INSERT ON CONFLICT DO NOTHING statement failed 
with segmentaion fault.
If the primary key constraint was not created on the partition, this statement 
executed successfully.

- Test
postgres=> CREATE TABLE part1(c1 NUMERIC, c2 VARCHAR(10)) PARTITION BY RANGE 
(c1) ;
CREATE TABLE
postgres=> CREATE TABLE part1p1 PARTITION OF part1 FOR VALUES FROM (100) TO 
(200) ;
CREATE TABLE
postgres=> ALTER TABLE part1p1 ADD CONSTRAINT pk_part1p1 PRIMARY KEY (c1) ;
ALTER TABLE
postgres=> INSERT INTO part1 VALUES (100, 'init') ON CONFLICT DO NOTHING ;
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
!> \q

- Part of data/log/postgresql.log file 
2017-03-30 10:20:09.161 JST [12323] LOG:  server process (PID 12337) was 
terminated by signal 11: Segmentation fault
2017-03-30 10:20:09.161 JST [12323] DETAIL:  Failed process was running: INSERT 
INTO part1 VALUES (100, 'init') ON CONFLICT DO NOTHING ;
2017-03-30 10:20:09.161 JST [12323] LOG:  terminating any other active server 
processes
2017-03-30 10:20:09.163 JST [12345] FATAL:  the database system is in recovery 
mode
2017-03-30 10:20:09.164 JST [12329] WARNING:  terminating connection because of 
crash of another server process
2017-03-30 10:20:09.164 JST [12329] DETAIL:  The postmaster has commanded this 
server process to roll back the current transaction and exit, because another 
server process exited abnormally and possibly corrupted shared memory.

- Environment
OS: Red Hat Enterprise Linux 7 Update 1 (x86-64) 
Snapshot: 2017-03-29 20:30:05 with default configure.

Best Regards,

--
Noriyoshi Shinoda

-Original Message-
From: pgsql-hackers-ow...@postgresql.org 
[mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Amit Langote
Sent: Tuesday, March 28, 2017 9:56 AM
To: Robert Haas 
Cc: Peter Geoghegan ; Simon Riggs ; 
PostgreSQL Hackers ; Thom Brown 
Subject: Re: [HACKERS] Partitioning vs ON CONFLICT

On 2017/03/27 23:40, Robert Haas wrote:
> On Thu, Mar 9, 2017 at 7:20 PM, Amit Langote 
>  wrote:
>> On 2017/03/10 9:10, Amit Langote wrote:
>>> On 2017/03/09 23:25, Robert Haas wrote:
 On Fri, Feb 17, 2017 at 1:47 AM, Amit Langote wrote:
> I updated the patch.  Now it's reduced to simply removing the 
> check in
> transformInsertStmt() that prevented using *any* ON CONFLICT on 
> partitioned tables at all.

 This patch no longer applies.
>>>
>>> Rebased patch is attached.
>>
>> Oops, really attached this time,
> 
> Committed with a bit of wordsmithing of the documentation.

Thanks.

Regards,
Amit

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Page Scan Mode in Hash Index

[Ashutosh Sharma]

I think you should consider refactoring this so that it doesn't need
>> to use goto.  Maybe move the while (offnum <= maxoff) logic into a
>> helper function and have it return itemIndex.  If itemIndex == 0, you
>> can call it again.
>>
>
> okay, Added a helper function for _hash_readpage(). Please check v4
> patch attached with this mail.
>
> >>

> This is not a full review, but I'm out of time for the moment.
>>
>
> No worries. I will be ready for your further review comments any time.
> Thanks for the review.
>
>
This patch needs a rebase.


Please try applying these patches on top of [1]. I think you should be able
to apply it cleanly. Sorry, I think I forgot to mention this point in my
earlier mail.

[1] -
https://www.postgresql.org/message-id/CAE9k0P%3DV2LhtyeMXd295fhisp%3DNWUhRVJ9EZQCDowWiY9rSohQ%40mail.gmail.com

Re: [HACKERS] PATCH: Batch/pipelining support for libpq

[Michael Paquier]

On Wed, Mar 29, 2017 at 12:40 PM, Vaishnavi Prabakaran
 wrote:
> Michael Paquier wrote:
>>Could you as well update src/tools/msvc/vcregress.pl, aka the routine
>>modulescheck so as this new test is skipped. I am sure that nobody
>>will scream if this test is not run on Windows, but the buildfarm will
>>complain if the patch is let in its current state.
>
> Thanks for the finding. Hmm, modulescheck will not pick up test_libpq as
> "subdircheck" will fail for this module(because it does not have
> "sql"/"expected" folders in it) and hence it will be skipped.
> But, Modified "Mkvcbuild.pm" to exclude "test_libpq" module, as this test
> anyways will not be run in Windows and also because it uses linux specific
> APIs(gettimeofday , timersub).

src/port/gettimeofday.c extends things on Windows, and we could
consider to just drop the timing portion for simplicity (except
Windows I am not seeing any platform missing timersub). But that's
just a point of detail. Or we could just drop those tests from the
final patch, and re-implement them after having some psql-level
subcommands. That would far better for portability.

> There are 2 cases tested here:
>
> 1. Example for the case - Using COPY command in batch mode will trigger
> failure. (Specified in documentation)
> Failure can be seen only when processing the copy command's results. That
> is, after dispatching COPY command, server goes into COPY state , but the
> client dispatched another query in batch mode.
>
> Below error messages belongs to this case :
> Error status and message got from server due to COPY command failure are :
> PGRES_FATAL_ERROR ERROR:  unexpected message type 0x50 during COPY from
> stdin
> CONTEXT:  COPY batch_demo, line 1
>
> message type 0x5a arrived from server while idle

Such messages are really confusing to the user as they refer to the
protocol going out of sync. I would think that something like "cannot
process COPY results during a batch processing" would be cleaner.
Isn't some more error handling necessary in GetCopyStart()?

> Attached the latest code and test patches.

For me the test is still broken:
ok 1 - testlibpqbatch disallowed_in_batch
ok 2 - testlibpqbatch simple_batch
ok 3 - testlibpqbatch multi_batch
ok 4 - testlibpqbatch batch_abort
ok 5 - testlibpqbatch timings
not ok 6 - testlibpqbatch copyfailure

Still broken here. I can see that this patch is having enough
safeguards in PQbatchBegin() and PQsendQueryStart(), but this issue is
really pointing out at something...
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

[Haribabu Kommi]

On Wed, Mar 29, 2017 at 11:04 PM, Andreas Karlsson 
wrote:

> On 03/29/2017 05:43 AM, Haribabu Kommi wrote:
> > Updated patch attached.
>
> I get a test failure in the pg_upgrade tests, but I do not have time right
> now to investigate.
>
> The failing test is "Restoring database schemas in the new cluster".
>

Thanks for test.

I found the reason for failure.

Before this refactor patch, in case of --binary-upgrade, the pg_dumpall
dumps all the global objects and also the database objects. These objects
will be restored first during the preparation of the new cluster and later
each individual database is restored.

Because of the refactoring of the database objects, currently as part of
globals dump with --binary-upgrade, no database objects gets dumped.
During restore no databases are created. so while restoring individual
database, it leads to failure as it not able to connect to the target
database.

Currently I marked the patch in the commitfest as "returned with feedback"
as in the current situation, this needs some analysis in handling database
objects in --binary-upgrade mode.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] [BUGS] Bug in Physical Replication Slots (at least 9.5)?

[Tatsuo Ishii]

> On Thu, Mar 30, 2017 at 9:12 AM, Tatsuo Ishii  wrote:
>> Committers will not apply patches which has trailing whitespace
>> issues. So the patch submitter needs to fix them anyway.
> 
> I cannot comment on that point (committers are free to pick up things
> the way they want), but just using git commands to apply a patch
> should not be an obstacle for a review if a patch can be easily
> applied as long as they roughly respect GNU's diff format.

My point is, the coding standard. Having trainling whitespace is
against our coding standard and committers should not accept such a
code, I believe.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [BUGS] Bug in Physical Replication Slots (at least 9.5)?

[Michael Paquier]

On Thu, Mar 30, 2017 at 9:12 AM, Tatsuo Ishii  wrote:
> Committers will not apply patches which has trailing whitespace
> issues. So the patch submitter needs to fix them anyway.

I cannot comment on that point (committers are free to pick up things
the way they want), but just using git commands to apply a patch
should not be an obstacle for a review if a patch can be easily
applied as long as they roughly respect GNU's diff format.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [BUGS] Bug in Physical Replication Slots (at least 9.5)?

[Venkata B Nagothi]

On Thu, Mar 30, 2017 at 10:55 AM, Michael Paquier  wrote:

> On Thu, Mar 30, 2017 at 8:49 AM, Venkata B Nagothi 
> wrote:
> > On Tue, Mar 28, 2017 at 5:51 PM, Kyotaro HORIGUCHI
> >  wrote:
> > I tried applying this patch to latest master, it is not getting applied
> >
> > [dba@buildhost postgresql]$ git apply
> > /data/postgresql-patches/9.5-ReplicationSlots-Bug-Patch/
> 0001-Fix-a-bug-of-physical-replication-slot_a6f22e8.patch
> > /data/postgresql-patches/9.5-ReplicationSlots-Bug-Patch/
> 0001-Fix-a-bug-of-physical-replication-slot_a6f22e8.patch:28:
> > trailing whitespace.
> > /*
> > /data/postgresql-patches/9.5-ReplicationSlots-Bug-Patch/
> 0001-Fix-a-bug-of-physical-replication-slot_a6f22e8.patch:29:
> > trailing whitespace.
> >  * This variable corresponds to restart_lsn in pg_replication_slots for a
> > /data/postgresql-patches/9.5-ReplicationSlots-Bug-Patch/
> 0001-Fix-a-bug-of-physical-replication-slot_a6f22e8.patch:30:
> > trailing whitespace.
> >  * physical slot. This has a valid value only when it differs from the
> > current
> > /data/postgresql-patches/9.5-ReplicationSlots-Bug-Patch/
> 0001-Fix-a-bug-of-physical-replication-slot_a6f22e8.patch:31:
> > trailing whitespace.
> >  * flush pointer.
> > /data/postgresql-patches/9.5-ReplicationSlots-Bug-Patch/
> 0001-Fix-a-bug-of-physical-replication-slot_a6f22e8.patch:32:
> > trailing whitespace.
> >  */
> > error: patch failed: src/backend/replication/walsender.c:210
> > error: src/backend/replication/walsender.c: patch does not apply
>
> git apply and git am can be very picky sometimes, so you may want to
> fallback to patch -p1 if things don't work. In this case it does.
>

patch -p1 seems to be working. Thanks !

Regards,

Venkata B N
Database Consultant

Re: [HACKERS] [BUGS] Bug in Physical Replication Slots (at least 9.5)?

[Tatsuo Ishii]

> On Thu, Mar 30, 2017 at 8:49 AM, Venkata B Nagothi  wrote:
>> On Tue, Mar 28, 2017 at 5:51 PM, Kyotaro HORIGUCHI
>>  wrote:
>> I tried applying this patch to latest master, it is not getting applied
>>
>> [dba@buildhost postgresql]$ git apply
>> /data/postgresql-patches/9.5-ReplicationSlots-Bug-Patch/0001-Fix-a-bug-of-physical-replication-slot_a6f22e8.patch
>> /data/postgresql-patches/9.5-ReplicationSlots-Bug-Patch/0001-Fix-a-bug-of-physical-replication-slot_a6f22e8.patch:28:
>> trailing whitespace.
>> /*
>> /data/postgresql-patches/9.5-ReplicationSlots-Bug-Patch/0001-Fix-a-bug-of-physical-replication-slot_a6f22e8.patch:29:
>> trailing whitespace.
>>  * This variable corresponds to restart_lsn in pg_replication_slots for a
>> /data/postgresql-patches/9.5-ReplicationSlots-Bug-Patch/0001-Fix-a-bug-of-physical-replication-slot_a6f22e8.patch:30:
>> trailing whitespace.
>>  * physical slot. This has a valid value only when it differs from the
>> current
>> /data/postgresql-patches/9.5-ReplicationSlots-Bug-Patch/0001-Fix-a-bug-of-physical-replication-slot_a6f22e8.patch:31:
>> trailing whitespace.
>>  * flush pointer.
>> /data/postgresql-patches/9.5-ReplicationSlots-Bug-Patch/0001-Fix-a-bug-of-physical-replication-slot_a6f22e8.patch:32:
>> trailing whitespace.
>>  */
>> error: patch failed: src/backend/replication/walsender.c:210
>> error: src/backend/replication/walsender.c: patch does not apply
> 
> git apply and git am can be very picky sometimes, so you may want to
> fallback to patch -p1 if things don't work. In this case it does.

Committers will not apply patches which has trailing whitespace
issues. So the patch submitter needs to fix them anyway.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] TPC-H Q20 from 1 hour to 19 hours!

[Tomas Vondra]

On 03/30/2017 12:14 AM, Tomas Vondra wrote:


I've only ran the queries on 10GB data set, but that should be enough. 
The plans are from current master - I'll rerun the script on an older 
release later today.




So, an plans from an older release (9.4) are attached. What seems to 
matter is the join between partsupp and part, which is estimated like 
this on 9.4:


   ->  Sort  (cost=172852.06..173741.94 rows=355951 width=12)
 (actual time=321.998..334.440 rows=86836 loops=1)
 Sort Key: partsupp.ps_partkey, partsupp.ps_suppkey
 Sort Method: quicksort  Memory: 7143kB
 ->  Nested Loop  (cost=0.43..140031.03 rows=355951 width=12)
  (actual time=0.025..303.145 rows=86836 loops=1)

and like this on current master:

  ->  Sort  (cost=146617.86..146819.89 rows=80809 width=12)
(actual time=562.513..575.599 rows=86836 loops=1)
 Sort Key: partsupp.ps_partkey, partsupp.ps_suppkey
 Sort Method: quicksort  Memory: 7143kB
 ->  Nested Loop  (cost=0.43..140031.03 rows=80809 width=12)
 (actual time=0.054..536.003 rows=86836 loops=1)

which however seems clearly more accurate than 9.4. So perhaps there is 
some bug in computing the mergejoin estimate, but it's also possible 
correcting the estimate (lowering it) also has some impact.


But joining to the aggregated subquery also clearly plays some role, 
because without it the estimates are higher.


Another interesting observation is that only the foreign key between 
part/partsupp seems to matter - once it gets dropped, the estimates get 
back close to 9.4 values.


What is however strange is that changing max_parallel_workers_per_gather 
affects row estimates *above* the Gather node. That seems a bit, um, 
suspicious, no? See the parallel-estimates.log.


regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
tpch=# \d lineitem;
 Table "public.lineitem"
 Column  | Type  | Collation | Nullable | Default 
-+---+---+--+-
 l_orderkey  | integer   |   | not null | 
 l_partkey   | integer   |   | not null | 
 l_suppkey   | integer   |   | not null | 
 l_linenumber| integer   |   | not null | 
 l_quantity  | numeric   |   |  | 
 l_extendedprice | numeric   |   |  | 
 l_discount  | numeric   |   |  | 
 l_tax   | numeric   |   |  | 
 l_returnflag| character(1)  |   |  | 
 l_linestatus| character(1)  |   |  | 
 l_shipdate  | date  |   |  | 
 l_commitdate| date  |   |  | 
 l_receiptdate   | date  |   |  | 
 l_shipinstruct  | character varying(25) |   |  | 
 l_shipmode  | character varying(10) |   |  | 
 l_comment   | character varying(44) |   |  | 
Indexes:
"lineitem_pkey" PRIMARY KEY, btree (l_orderkey, l_linenumber)
"idx_lineitem_orderkey" btree (l_orderkey)
"idx_lineitem_part_supp" btree (l_partkey, l_suppkey)
"idx_lineitem_shipdate" btree (l_shipdate, l_discount, l_quantity)
Foreign-key constraints:
"lineitem_l_orderkey_fkey" FOREIGN KEY (l_orderkey) REFERENCES orders(o_orderkey)
"lineitem_l_partkey_fkey" FOREIGN KEY (l_partkey, l_suppkey) REFERENCES partsupp(ps_partkey, ps_suppkey)

tpch=# \d part
  Table "public.part"
Column | Type  | Collation | Nullable | Default 
---+---+---+--+-
 p_partkey | integer   |   | not null | nextval('part_p_partkey_seq'::regclass)
 p_name| character varying(55) |   |  | 
 p_mfgr| character(25) |   |  | 
 p_brand   | character(10) |   |  | 
 p_type| character varying(25) |   |  | 
 p_size| integer   |   |  | 
 p_container   | character(10) |   |  | 
 p_retailprice | numeric   |   |  | 
 p_comment | character varying(23) |   |  | 
Indexes:
"part_pkey" PRIMARY KEY, btree (p_partkey)

tpch=# \d partsupp
 Table "public.partsupp"
Column |  Type  | Collation | Nullable | Default 
---++---+--+-
 ps_partkey| integer|   | not null | 
 

Re: [HACKERS] [BUGS] Bug in Physical Replication Slots (at least 9.5)?

[Michael Paquier]

On Thu, Mar 30, 2017 at 8:49 AM, Venkata B Nagothi  wrote:
> On Tue, Mar 28, 2017 at 5:51 PM, Kyotaro HORIGUCHI
>  wrote:
> I tried applying this patch to latest master, it is not getting applied
>
> [dba@buildhost postgresql]$ git apply
> /data/postgresql-patches/9.5-ReplicationSlots-Bug-Patch/0001-Fix-a-bug-of-physical-replication-slot_a6f22e8.patch
> /data/postgresql-patches/9.5-ReplicationSlots-Bug-Patch/0001-Fix-a-bug-of-physical-replication-slot_a6f22e8.patch:28:
> trailing whitespace.
> /*
> /data/postgresql-patches/9.5-ReplicationSlots-Bug-Patch/0001-Fix-a-bug-of-physical-replication-slot_a6f22e8.patch:29:
> trailing whitespace.
>  * This variable corresponds to restart_lsn in pg_replication_slots for a
> /data/postgresql-patches/9.5-ReplicationSlots-Bug-Patch/0001-Fix-a-bug-of-physical-replication-slot_a6f22e8.patch:30:
> trailing whitespace.
>  * physical slot. This has a valid value only when it differs from the
> current
> /data/postgresql-patches/9.5-ReplicationSlots-Bug-Patch/0001-Fix-a-bug-of-physical-replication-slot_a6f22e8.patch:31:
> trailing whitespace.
>  * flush pointer.
> /data/postgresql-patches/9.5-ReplicationSlots-Bug-Patch/0001-Fix-a-bug-of-physical-replication-slot_a6f22e8.patch:32:
> trailing whitespace.
>  */
> error: patch failed: src/backend/replication/walsender.c:210
> error: src/backend/replication/walsender.c: patch does not apply

git apply and git am can be very picky sometimes, so you may want to
fallback to patch -p1 if things don't work. In this case it does.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [BUGS] Bug in Physical Replication Slots (at least 9.5)?

[Venkata B Nagothi]

Regards,

Venkata B N
Database Consultant


On Tue, Mar 28, 2017 at 5:51 PM, Kyotaro HORIGUCHI <
horiguchi.kyot...@lab.ntt.co.jp> wrote:

> This conflicts with 6912acc (replication lag tracker) so just
> rebased on a6f22e8.
>

I tried applying this patch to latest master, it is not getting applied

[dba@buildhost postgresql]$ git apply
/data/postgresql-patches/9.5-ReplicationSlots-Bug-Patch/0001-Fix-a-bug-of-physical-replication-slot_a6f22e8.patch
/data/postgresql-patches/9.5-ReplicationSlots-Bug-Patch/0001-Fix-a-bug-of-physical-replication-slot_a6f22e8.patch:28:
trailing whitespace.
/*
/data/postgresql-patches/9.5-ReplicationSlots-Bug-Patch/0001-Fix-a-bug-of-physical-replication-slot_a6f22e8.patch:29:
trailing whitespace.
 * This variable corresponds to restart_lsn in pg_replication_slots for a
/data/postgresql-patches/9.5-ReplicationSlots-Bug-Patch/0001-Fix-a-bug-of-physical-replication-slot_a6f22e8.patch:30:
trailing whitespace.
 * physical slot. This has a valid value only when it differs from the
current
/data/postgresql-patches/9.5-ReplicationSlots-Bug-Patch/0001-Fix-a-bug-of-physical-replication-slot_a6f22e8.patch:31:
trailing whitespace.
 * flush pointer.
/data/postgresql-patches/9.5-ReplicationSlots-Bug-Patch/0001-Fix-a-bug-of-physical-replication-slot_a6f22e8.patch:32:
trailing whitespace.
 */
error: patch failed: src/backend/replication/walsender.c:210
error: src/backend/replication/walsender.c: patch does not apply


Regards,

Venkata Balaji N
Database Consultant

Re: [HACKERS] [PATCH] Reduce src/test/recovery verbosity

[Michael Paquier]

On Thu, Mar 30, 2017 at 4:08 AM, Stephen Frost  wrote:
> * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
>> Stephen Frost wrote:
>> > * Dagfinn Ilmari Mannsåker (ilm...@ilmari.org) wrote:
>>
>> > > Non-verbose prove still lists each test script, it just doesn't list
>> > > each individual test within the script.
>> >
>> > I agree that it'd be better to just show the per-script results rather
>> > than every little test result because it's just too much.
>>
>> Well, for some test files a single test could take half a minute.  A
>> dozen lines taking in total about a minute seems fine to me.  The
>> problem I see is a (pg_dump I think) script that has over 500 tests.
>
> Not sure that I see that as a "problem". :)
>
>> > If there's a way to change the verbosity for just those scripts, I'd be
>> > happy to do that, if we're unable to agree on reducing it across the
>> > board..
>>
>> I'd rather silence only scripts that are overly verbose.
>
> I'm fine with that, but how?

The important point to me is not to show the tests that passed, it is
to show the tests that are failing when running them. So I would
suggest to just remove the --verbose flag in PROVE_FLAGS. If you do
so, the test of pg_dump would show up like that, printing as well a
count number while running:
t/001_basic.pl . ok
t/002_pg_dump.pl ... ok
t/010_dump_connstr.pl .. ok

And failures would still show up, here is an example breaking manually
a recovery test:
t/001_stream_rep.pl .. 1/28
#   Failed test 'read-only queries on standby 2'
#   at t/001_stream_rep.pl line 59.
#  got: '3'
# expected: '0'
# testing connection parameter "target_session_attrs"
# switching to physical replication slot
t/001_stream_rep.pl .. 11/28 # enabling hot_standby_feedback
t/001_stream_rep.pl .. 15/28 # doing some work to advance xmin
# new xmin 1442, old xmin 558
t/001_stream_rep.pl .. 19/28 # new xmin 1553, old xmin 558
# disabling hot_standby_feedback
t/001_stream_rep.pl .. 23/28 # re-enabling
hot_standby_feedback and disabling while stopped
t/001_stream_rep.pl .. 28/28 # Looks like you failed 1
test of 28.
t/001_stream_rep.pl .. Dubious, test returned 1 (wstat
256, 0x100)
Failed 1/28 subtests
t/002_archiving.pl ... ok
t/003_recovery_targets.pl  ok
t/004_timeline_switch.pl . ok

Attached is a patch.
-- 
Michael


tap-less-verbose.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication access control patches

[Petr Jelinek]

On 29/03/17 20:55, Alvaro Herrera wrote:
> Peter Eisentraut wrote:
>> On 3/15/17 21:54, Peter Eisentraut wrote:
> 
>>> 0004 Add subscription apply worker privilege checks
>>> 0005 Add CREATE SUBSCRIPTION privilege on databases
>>
>> It would be nice to reach a conclusion on these (the second one
>> particularly), because otherwise we'll be stuck with only superusers
>> being allowed to create subscriptions.
> 
> I note that the CREATE privilege on databases, which previously only
> enabled schema creation, now also allows to create publications.  I
> wonder what is different about subscriptions that we need a separate
> CREATE SUBSCRIPTION privilege; could we allow the three things under the
> same privilege type?  (I suspect not; why give logical replication
> controls to users who in previous releases were only able to create
> schemas?)  If not, does it make sense to have one privilege for both new
> things, perhaps something like GRANT LOGICAL REPLICATION THINGIES?  If
> not, maybe we should have three separate priv bits: GRANT CREATE for
> schemas, GRANT CREATE PUBLICATION and GRANT CREATE SUBSCRIPTION?
> 
> 
> So this CREATE SUBSCRIPTION priv actually gives you the power to cause
> the system to open network connections to the outside world.  It's not
> something you give freely to random strangers -- should be guarded
> moderately tight, because it could be used as covert channel for data
> leaking.  However, it's 1000x better than requiring superuser for
> subscription creation, so +1 for the current approach.
> 

Plus on the other hand you might want to allow somebody to stream data
from another server but not necessarily allow said person to create new
objects in the database which standard CREATE privilege would allow. So
I think it makes sense to push this approach.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] TPC-H Q20 from 1 hour to 19 hours!

[Tomas Vondra]

On 03/29/2017 09:00 PM, Robert Haas wrote:

On Mon, Mar 6, 2017 at 1:22 AM, Rafia Sabih
 wrote:

This is to bring to notice a peculiar instance I found recently while
running TPC-H benchmark queries. Q20 of the benchmark took 19 hours to
complete ...


That's bad.


It is clear that selectivity estimations are really bad in this case
particularly at node,
   ->  Merge Join  (cost=52959586.72..60024468.82 rows=85 width=16)
(actual time=1525322.753..2419045.641 rows=1696742 loops=1)
Merge Cond: ((lineitem.l_partkey =
partsupp.ps_partkey) AND (lineitem.l_suppkey = partsupp.ps_suppkey))
Join Filter:
((partsupp.ps_availqty)::numeric > ((0.5 * sum(lineitem.l_quantity
Rows Removed by Join Filter: 3771


So, the selectivity estimation here is bad both before and after Tom's
commit, but it's significantly worse after (actual value 1696742, old
estimate 3771, new estimate 85).


Still this puzzled me as during earlier runs of this benchmark I never
encountered such prolonged running times. On further investigation I
found that on reverting the commit
7fa93eec4e0c9c3e801e3c51aa4bae3a38aaa218
Author: Tom Lane 
Date:   Sat Dec 17 15:28:54 2016 -0500
 Fix FK-based join selectivity estimation for semi/antijoins.


I don't think the problem originates at the Merge Join, though,
because the commit says that at is fixing semi and anti-join estimates
- this is a plain inner join, so in theory it shouldn't change.
However, it's a bit hard for me to piece through these plans, the
formatting kind of got messed up - things are wrapped.  Could you
possibly attach the plans as attachments?



I've been looking into this today, and it seems to me the simplest query 
triggering this issue (essentially a part of q20) is this:


select
ps_suppkey
from
partsupp,
(
select
l_partkey agg_partkey,
l_suppkey agg_suppkey
from
lineitem
group by
l_partkey,
l_suppkey
) agg_lineitem
where
agg_partkey = ps_partkey
and agg_suppkey = ps_suppkey
and ps_partkey in (
select
p_partkey
from
part
where
p_name like 'hot%'
);

which does actually include a semijoin. What seems to trigger the issue 
is the join to the aggregated lineitem table - when replacing it with a 
plain table, everything seems to be estimated perfectly fine.


Attached is a simple SQL script, that runs three variants of the query:

(1) with the join to the aggregated lineitem table
(2) with a join to a plain lineitem table
(3) with a join to a plain lineitem table and to 'part' (without the 
semijoin)


First the queries are executed on tables without any foreign keys 
(between those three), then with a FK between lineitem and partsupp, and 
finally with additional FK between partsupp and part.


Initially the estimates are bad, but once the first foreign key is 
added, the estimates get very accurate - except for the case (1).


I've only ran the queries on 10GB data set, but that should be enough. 
The plans are from current master - I'll rerun the script on an older 
release later today.


regards

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


q20.sql
Description: application/sql
set max_parallel_workers_per_gather = 0;
SET
 Table "public.lineitem"
 Column  | Type  | Collation | Nullable | Default 
-+---+---+--+-
 l_orderkey  | integer   |   | not null | 
 l_partkey   | integer   |   | not null | 
 l_suppkey   | integer   |   | not null | 
 l_linenumber| integer   |   | not null | 
 l_quantity  | numeric   |   |  | 
 l_extendedprice | numeric   |   |  | 
 l_discount  | numeric   |   |  | 
 l_tax   | numeric   |   |  | 
 l_returnflag| character(1)  |   |  | 
 l_linestatus| character(1)  |   |  | 
 l_shipdate  | date  |   |  | 
 l_commitdate| date  |   |  | 
 l_receiptdate   | date  |   |  | 
 l_shipinstruct  | character varying(25) |   |  | 
 l_shipmode  | character varying(10) |   |  | 
 l_comment   | character varying(44) |   |  | 
Indexes:
"lineitem_pkey" PRIMARY 

Re: [HACKERS] Page Scan Mode in Hash Index

[Jesper Pedersen]

Hi,

On 03/27/2017 09:34 AM, Ashutosh Sharma wrote:

Hi,


I think you should consider refactoring this so that it doesn't need
to use goto.  Maybe move the while (offnum <= maxoff) logic into a
helper function and have it return itemIndex.  If itemIndex == 0, you
can call it again.


okay, Added a helper function for _hash_readpage(). Please check v4
patch attached with this mail.


>>

This is not a full review, but I'm out of time for the moment.


No worries. I will be ready for your further review comments any time.
Thanks for the review.



This patch needs a rebase.

Best regards,
 Jesper



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

New Patch v29: Now with less coverage!


Patch applies cleanly. Make check ok. Feature still works!

--
Fabien.


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Monitoring roles patch

[Dave Page]

On Wed, Mar 29, 2017 at 2:51 PM, Stephen Frost  wrote:
>
> Dave's currently hacking on a new patch based on our discussion, so I'd
> suggest waiting another hour or so anyway until he's done.
>
> Might be a bit longer as he's trying to do it in a hallway at
> PGConf.US...

Thanks Stephen.

Here's an updated patch, and description of the changes. Simon,
Stephen and Robert have looked at the description and are all happy
with it \o/. Thank you to them for taking the time out of the
conference to go through it with me.

Here's what it does:

1) Creates the following default roles:

  - pg_monitor - Top-level role that is GRANTed all of the following
roles by default. Also GRANTed access to some additional functions.
  - pg_read_all_settings - A role that can read all GUCs.
  - pg_read_all_stats - A role that can read un-redacted pg_stat_*
views via the functions supporting them, as well as
pg_database_size/pg_tablespace_size.
  - pg_stat_scan_tables - A role that can execute monitoring functions
that may lock tables.

2) pg_database_size and pg_tablespace_size have hard-coded permission
checks updated to allow execution by pg_read_all_stats.

3) GUC read permission checks for superuser have been replaced with
checks for membership in pg_read_all_settings.

4) pg_buffercache functions have GRANTed execute permissions to pg_monitor.

5) pg_freespacemap functions have GRANTed execute permissions to
pg_stat_scan_tables.

6) pg_stat_statements has its hard-coded permission check updated to
allow execution by pg_read_all_stats, and the same role is GRANTed
permission to execute pg_stat_statements_reset().

7) pg_visibility functions have GRANTed executed permissions to
pg_stat_scan_tables.

8) pgrowlocks has it's hard-coded permission check updated to allow
execution by pg_stat_scan_tables,

9) pgstattuple functions have GRANTed executed permissions to
pg_stat_scan_tables.

10) pg_stat_get_wal_receiver has its hard-coded permission check
updated to allow execution by pg_read_all_stats

11) pg_ls_logdir and pg_ls_waldir have execute permissions GRANTed to pg_monitor

12) Un-redacted use of the functions underpinning the pg_stat_* views
is available to pg_read_all_stats.


-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/contrib/pg_buffercache/Makefile b/contrib/pg_buffercache/Makefile
index 497dbeb229..18f7a87452 100644
--- a/contrib/pg_buffercache/Makefile
+++ b/contrib/pg_buffercache/Makefile
@@ -4,8 +4,9 @@ MODULE_big = pg_buffercache
 OBJS = pg_buffercache_pages.o $(WIN32RES)
 
 EXTENSION = pg_buffercache
-DATA = pg_buffercache--1.2.sql pg_buffercache--1.1--1.2.sql \
-   pg_buffercache--1.0--1.1.sql pg_buffercache--unpackaged--1.0.sql
+DATA = pg_buffercache--1.2.sql pg_buffercache--1.2--1.3.sql \
+   pg_buffercache--1.1--1.2.sql pg_buffercache--1.0--1.1.sql \
+   pg_buffercache--unpackaged--1.0.sql
 PGFILEDESC = "pg_buffercache - monitoring of shared buffer cache in real-time"
 
 ifdef USE_PGXS
diff --git a/contrib/pg_buffercache/pg_buffercache--1.2--1.3.sql 
b/contrib/pg_buffercache/pg_buffercache--1.2--1.3.sql
new file mode 100644
index 00..b37ef0112e
--- /dev/null
+++ b/contrib/pg_buffercache/pg_buffercache--1.2--1.3.sql
@@ -0,0 +1,7 @@
+/* contrib/pg_buffercache/pg_buffercache--1.2--1.3.sql */
+
+-- complain if script is sourced in psql, rather than via ALTER EXTENSION
+\echo Use "ALTER EXTENSION pg_buffercache UPDATE TO '1.3'" to load this file. 
\quit
+
+GRANT EXECUTE ON FUNCTION pg_buffercache_pages() TO pg_monitor;
+GRANT SELECT ON pg_buffercache TO pg_monitor;
diff --git a/contrib/pg_buffercache/pg_buffercache.control 
b/contrib/pg_buffercache/pg_buffercache.control
index a4d664f3fa..8c060ae9ab 100644
--- a/contrib/pg_buffercache/pg_buffercache.control
+++ b/contrib/pg_buffercache/pg_buffercache.control
@@ -1,5 +1,5 @@
 # pg_buffercache extension
 comment = 'examine the shared buffer cache'
-default_version = '1.2'
+default_version = '1.3'
 module_pathname = '$libdir/pg_buffercache'
 relocatable = true
diff --git a/contrib/pg_freespacemap/Makefile b/contrib/pg_freespacemap/Makefile
index 7bc0e9555d..0a2f000ec6 100644
--- a/contrib/pg_freespacemap/Makefile
+++ b/contrib/pg_freespacemap/Makefile
@@ -4,8 +4,8 @@ MODULE_big = pg_freespacemap
 OBJS = pg_freespacemap.o $(WIN32RES)
 
 EXTENSION = pg_freespacemap
-DATA = pg_freespacemap--1.1.sql pg_freespacemap--1.0--1.1.sql \
-   pg_freespacemap--unpackaged--1.0.sql
+DATA = pg_freespacemap--1.1.sql pg_freespacemap--1.1--1.2.sql \
+   pg_freespacemap--1.0--1.1.sql pg_freespacemap--unpackaged--1.0.sql
 PGFILEDESC = "pg_freespacemap - monitoring of free space map"
 
 ifdef USE_PGXS
diff --git a/contrib/pg_freespacemap/pg_freespacemap--1.1--1.2.sql 
b/contrib/pg_freespacemap/pg_freespacemap--1.1--1.2.sql
new file mode 100644
index 00..f558defadd
--- /dev/null
+++ 

Re: [HACKERS] [PATCH] SortSupport for macaddr type

[Teodor Sigaev]

Thank you, pushed

Excellent! I've attached a new (and hopefully final)
version of the patch.


--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] few fts functions for jsonb

[Dmitry Dolgov]

> On 29 March 2017 at 18:28, Andrew Dunstan 
wrote:
>
> These patches seem fundamentally OK. But I'm still not happy with the
> naming etc.

I've changed names for all functions and action definitions, moved out the
changes in header file to `jsonapi.h` and removed `is_jsonb_data` macro. So
it
should be better now.
diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index 6a7aab2..c9f86b0 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -52,6 +52,25 @@ typedef struct OkeysState
 	int			sent_count;
 } OkeysState;
 
+/* state for iterate_json function */
+typedef struct IterateJsonState
+{
+	JsonLexContext	*lex;
+	JsonIterateStringValuesAction	action;			/* an action that will be applied
+	   to each json value */
+	void			*action_state;	/* any necessary context for iteration */
+} IterateJsonState;
+
+/* state for transform_json function */
+typedef struct TransformJsonState
+{
+	JsonLexContext	*lex;
+	StringInfo		strval;			/* resulting json */
+	JsonTransformStringValuesAction	action;			/* an action that will be applied
+	   to each json value */
+	void			*action_state;	/* any necessary context for transformation */
+} TransformJsonState;
+
 /* state for json_get* functions */
 typedef struct GetState
 {
@@ -271,6 +290,18 @@ static void setPathArray(JsonbIterator **it, Datum *path_elems,
 			 int level, Jsonb *newval, uint32 nelems, int op_type);
 static void addJsonbToParseState(JsonbParseState **jbps, Jsonb *jb);
 
+/* function supporting iterate_json(b) */
+static void iterate_string_values_scalar(void *state, char *token, JsonTokenType tokentype);
+
+/* function supporting transform_json(b) */
+static void transform_string_values_object_start(void *state);
+static void transform_string_values_object_end(void *state);
+static void transform_string_values_array_start(void *state);
+static void transform_string_values_array_end(void *state);
+static void transform_string_values_object_field_start(void *state, char *fname, bool isnull);
+static void transform_string_values_array_element_start(void *state, bool isnull);
+static void transform_string_values_scalar(void *state, char *token, JsonTokenType tokentype);
+
 
 /*
  * SQL function json_object_keys
@@ -4130,3 +4161,208 @@ setPathArray(JsonbIterator **it, Datum *path_elems, bool *path_nulls,
 		}
 	}
 }
+
+/*
+ * Iterate over jsonb string values or elements, and pass them together with an
+ * iteration state to a specified JsonIterateStringValuesAction.
+ */
+void
+iterate_jsonb_string_values(Jsonb *jb, void *state, JsonIterateStringValuesAction action)
+{
+	JsonbIterator		*it;
+	JsonbValue			v;
+	JsonbIteratorToken	type;
+
+	it = JsonbIteratorInit(>root);
+
+	while ((type = JsonbIteratorNext(, , false)) != WJB_DONE)
+	{
+		if ((type == WJB_VALUE || type == WJB_ELEM) && v.type == jbvString)
+		{
+			action(state, v.val.string.val, v.val.string.len);
+		}
+	}
+}
+
+/*
+ * Iterate over json string values or elements, and pass them together with an
+ * iteration state to a specified JsonIterateStringValuesAction.
+ */
+void
+iterate_json_string_values(text *json, void *action_state, JsonIterateStringValuesAction action)
+{
+	JsonLexContext *lex = makeJsonLexContext(json, true);
+	JsonSemAction *sem = palloc0(sizeof(JsonSemAction));
+	IterateJsonState   *state = palloc0(sizeof(IterateJsonState));
+
+	state->lex = lex;
+	state->action = action;
+	state->action_state = action_state;
+
+	sem->semstate = (void *) state;
+	sem->scalar = iterate_string_values_scalar;
+
+	pg_parse_json(lex, sem);
+}
+
+/*
+ * An auxiliary function for iterate_json_string_values to invoke a specified
+ * JsonIterateStringValuesAction.
+ */
+static void
+iterate_string_values_scalar(void *state, char *token, JsonTokenType tokentype)
+{
+	IterateJsonState   *_state = (IterateJsonState *) state;
+	if (tokentype == JSON_TOKEN_STRING)
+		(*_state->action) (_state->action_state, token, strlen(token));
+}
+
+/*
+ * Iterate over a jsonb, and apply a specified JsonTransformStringValuesAction
+ * to every string value or element. Any necessary context for a
+ * JsonTransformStringValuesAction can be passed in the action_state variable.
+ * Function returns a copy of an original jsonb object with transformed values.
+ */
+Jsonb *
+transform_jsonb_string_values(Jsonb *jsonb, void *action_state,
+			  JsonTransformStringValuesAction transform_action)
+{
+	JsonbIterator		*it;
+	JsonbValue			v, *res = NULL;
+	JsonbIteratorToken	type;
+	JsonbParseState		*st = NULL;
+	text*out;
+	boolis_scalar = false;
+
+	it = JsonbIteratorInit(>root);
+	is_scalar = it->isScalar;
+
+	while ((type = JsonbIteratorNext(, , false)) != WJB_DONE)
+	{
+		if ((type == WJB_VALUE || type == WJB_ELEM) && v.type == jbvString)
+		{
+			out = transform_action(action_state, v.val.string.val, v.val.string.len);
+			v.val.string.val = VARDATA_ANY(out);
+			

Re: [HACKERS] REINDEX CONCURRENTLY 2.0

[Michael Banck]

Hi,

I had a look at this.

On Mon, Mar 13, 2017 at 03:11:50AM +0100, Andreas Karlsson wrote:
> Spotted one of my TODO comments there so I have attached a patch where I
> have cleaned up that function. I also fixed the the code to properly support
> triggers.

The patch applies with quite a few offsets on top of current (2fd8685)
master, I have not verified that those are all ok.

Regression tests pass, also the included isolation tests.

I hope that Michael will post a full review as he worked on the code
extensively, but here are some some code comments, mostly on the
comments (note that I'm not a native speaker, so I might be wrong on
some of them as well):

> diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
> index 3908ade37b..3449c0af73 100644
> --- a/doc/src/sgml/ref/reindex.sgml
> +++ b/doc/src/sgml/ref/reindex.sgml
> @@ -68,9 +68,12 @@ REINDEX [ ( VERBOSE ) ] { INDEX | TABLE | SCHEMA | 
> DATABASE | SYSTEM } An index build with the CONCURRENTLY option failed, leaving
>an invalid index. Such indexes are useless but it can be
>convenient to use REINDEX to rebuild them. Note that
> -  REINDEX will not perform a concurrent build. To build the
> -  index without interfering with production you should drop the index and
> -  reissue the CREATE INDEX CONCURRENTLY command.
> +  REINDEX will perform a concurrent build if 
> +  CONCURRENTLY is specified. To build the index without interfering
> +  with production you should drop the index and reissue either the
> +  CREATE INDEX CONCURRENTLY or REINDEX 
> CONCURRENTLY
> +  command. Indexes of toast relations can be rebuilt with 
> REINDEX
> +  CONCURRENTLY.

I think the "To build the index[...]" part should be rephrased, the
current diff makes it sound like you should drop the index first even if
you reindex concurrently. What about "Note that REINDEX will
only perform a concurrent build if  CONCURRENTLY is
specified"?

Anyway, this part is only about reindexing invalid indexes, so
mentioning that reindex is not concurrently or the part about create-
index-concurrently-then-rename only for this case is a bit weird, but
this is a pre-existing condition.

> diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
> index 8d42a347ea..c40ac0b154 100644
> --- a/src/backend/catalog/index.c
> +++ b/src/backend/catalog/index.c
>  /*
> + * index_concurrent_create_copy
> + *
> + * Create a concurrent index based on the definition of the one provided by
> + * caller that will be used for concurrent operations. The index is inserted
> + * into catalogs and needs to be built later on. This is called during
> + * concurrent reindex processing. The heap relation on which is based the 
> index
> + * needs to be closed by the caller.
> + */

That should be "The heap relation on which the index is based ..." I
think.

> + /*
> +  * We have to re-build the IndexInfo struct, since it was lost in
> +  * commit of transaction where this concurrent index was created
> +  * at the catalog level.
> +  */
> + indexInfo = BuildIndexInfo(indexRelation);

Looks like indexInfo starts with lowercase, but the comment above has
upper case `IndexInfo'.

> +/*
> + * index_concurrent_swap
> + *
> + * Swap name, dependencies and constraints of the old index over to the new
> + * index, while marking the old index as invalid and the new as valid.
> + */

The `while' looks slightly odd to me, ISTM this is just another
operation this function performs, whereas "while" makes it sound like
the marking happens concurrently; so maybe ". Also, mark the old index
as invalid[...]"?

> +void
> +index_concurrent_swap(Oid newIndexOid, Oid oldIndexOid, const char *oldName)
> +{

[...]

> + /*
> +  * Copy contraint flags for old index. This is safe because the old 
> index
> +  * guaranteed uniquness.
> +  */

"uniqueness".

> + /* Mark old index as valid and new is invalid as index_set_state_flags 
> */

"new as invalid". Also, this comment style is different to this one:

> + /*
> +  * Move contstraints and triggers over to the new index
> +  */

I guess the latter could be changed to a one-line comment as the former,
but maybe there is a deeper sense (locality of comment?) in this.

> + /* make a modifiable copy */

I think comments should start capitalized?


> +/*
> + * index_concurrent_drop
> + *
> + * Drop a single index concurrently as the last step of an index concurrent
> + * process. Deletion is done through performDeletion or dependencies of the
> + * index would not get dropped. At this point all the indexes are already
> + * considered as invalid and dead so they can be dropped without using any
> + * concurrent options as it is sure that they will not interact with other
> + * server sessions.
> + */

I'd write "as it is certain" instead of "as it is sure", but I can't
explain why. Maybe persons are sure, but situations 

Re: [HACKERS] error handling in RegisterBackgroundWorker

[Alvaro Herrera]

Robert Haas wrote:
> On Wed, Mar 29, 2017 at 2:10 PM, Peter Eisentraut
>  wrote:
> > How specifically would we do that?  And what user would choose the
> > behavior "start this background worker but don't worry if it doesn't work"?
> 
> Well, if the background worker is auto-prewarm, you'd probably rather
> have the database start rather than get unhappy about auto-prewarm
> failing.  If the background worker is your logical replication
> launcher it's a bit more serious, but if you have no subscriptions or
> they're not that critical, maybe you don't care.  If the background
> worker is in charge of telling your failover solution that this node
> is up, then starting without it is entirely pointless.
> 
> I would be inclined to leave this alone for now and revisit it for a
> future release.  I don't feel confident that we really know what the
> right thing to do is here.

I think the common case is for modules to be critical: you may not care
about it for auto-prewarm, but that seems like a special case.  I would
put it the other way around: having the load fail is a serious problem
unless specifically configured not to be.  I'd do as Peter suggests, and
perhaps allow the current behavior optionally.  In hindsight, the
current behavior seems like a mistake.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Schedule and Release Management Team for PG10

[Alvaro Herrera]

Robert Haas wrote:
> On Wed, Mar 29, 2017 at 3:04 PM, Alvaro Herrera
>  wrote:

> > I was rather surprised to see the March commitfest declared to exactly
> > one month and feature freeze immediately thereafter.
> 
> That's true, but at
> https://wiki.postgresql.org/wiki/PgCon_2016_Developer_Meeting we
> agreed on March 31st as the feature freeze date.  I don't think the
> RMT should substantially vary a date that was agreed by a developer
> meeting with wide attendance; that seems like the tyranny of the few
> over the many.

Yes, I agree with that -- which is why I didn't comment when I read the
minutes several weeks ago.  I'm not proposing to change them, only
raising the point hoping that perhaps we'll choose differently next time.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

New Patch v29: Now with less coverage!
(same as v28 minus the psql-on-error-stop.sql and associated changes)
Fabien raises some good points about if/then being a tremendous tool for
enhancing other existing regression tests.


On Wed, Mar 29, 2017 at 2:16 PM, Fabien COELHO  wrote:

>
> Hello Tom,
>
> If someone were to put together a TAP test suite that covered all that
>> and made for a meaningful improvement in psql's altogether-miserable
>> code coverage report[1], I would think that that would be a useful
>> expenditure of buildfarm time.
>>
>
> Ok, this is an interesting point.
>
> What I'm objecting to is paying the overhead for such a suite in order to
>> test just this one thing.
>>
>
> Well, it should start somewhere. Once something is running it is easier to
> add more tests.
>
> think that that passes the bang-for-buck test; or in other words, this
>> isn't the place I would start if I were creating a TAP suite for psql.
>>
>
> Sure, I would not have started with that either.
>
> Note that from this patch point of view, it is somehow logical to start
> testing a given feature when this very feature is being developed...
>
> The summary is that we agree that psql test coverage is abysmal, but you
> do not want to bootstrap a better test infrastructure for this particular
> and rather special new feature. Ok.
>
> Maybe Corey can submit another patch with the exit 3 test removed.
>
> --
> Fabien.
>
From 9caa338fa085bc1c0ee16f22c0c1c8d3c8fc1697 Mon Sep 17 00:00:00 2001
From: Corey Huinker 
Date: Wed, 29 Mar 2017 15:30:22 -0400
Subject: [PATCH] psql if v29

---
 doc/src/sgml/ref/psql-ref.sgml |   90 ++-
 src/bin/psql/.gitignore|2 +
 src/bin/psql/Makefile  |4 +-
 src/bin/psql/command.c | 1365 
 src/bin/psql/common.c  |4 +-
 src/bin/psql/conditional.c |  103 +++
 src/bin/psql/conditional.h |   62 ++
 src/bin/psql/copy.c|4 +-
 src/bin/psql/help.c|7 +
 src/bin/psql/mainloop.c|   54 +-
 src/bin/psql/prompt.c  |6 +-
 src/bin/psql/prompt.h  |3 +-
 src/bin/psql/startup.c |8 +-
 src/test/regress/expected/psql.out |  110 +++
 src/test/regress/sql/psql.sql  |  109 +++
 15 files changed, 1641 insertions(+), 290 deletions(-)
 create mode 100644 src/bin/psql/conditional.c
 create mode 100644 src/bin/psql/conditional.h

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 2a9c412..18f8bfe 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2064,6 +2064,93 @@ hello 10
 
 
   
+\if expression
+\elif expression
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks.
+A conditional block must begin with an \if and end
+with an \endif.  In between there may be any number
+of \elif clauses, which may optionally be followed
+by a single \else clause.  Ordinary queries and
+other types of backslash commands may (and usually do) appear between
+the commands forming a conditional block.
+
+
+The \if and \elif commands read
+the rest of the line and evaluate it as a boolean expression.  If the
+expression is true then processing continues
+normally; otherwise, lines are skipped until a
+matching \elif, \else,
+or \endif is reached.  Once
+an \if or \elif has succeeded,
+later matching \elif commands are not evaluated but
+are treated as false.  Lines following an \else are
+processed only if no earlier matching \if
+or \elif succeeded.
+
+
+Lines being skipped are parsed normally to identify queries and
+backslash commands, but queries are not sent to the server, and
+backslash commands other than conditionals
+(\if, \elif,
+\else, \endif) are
+ignored.  Conditional commands are checked only for valid nesting.
+
+
+The expression argument
+of \if or \elif
+is subject to variable interpolation and backquote expansion, just
+like any other backslash command argument.  After that it is evaluated
+like the value of an on/off option variable.  So a valid value
+is any unambiguous case-insensitive match for one of:
+true, false, 1,
+0, on, off,
+yes, no.  For example,
+t, T, and tR
+will all be considered to be true.
+
+
+Expressions that do not properly evaluate to true or false will
+generate a warning and be treated as false.
+
+
+All the backslash commands of a given conditional block must appear in
+the same source file. If EOF is reached on 

Re: [HACKERS] Schedule and Release Management Team for PG10

[Dave Page]

On Wed, Mar 29, 2017 at 3:10 PM, Andres Freund  wrote:
> On 2017-03-29 16:04:50 -0300, Alvaro Herrera wrote:
>> Tom Lane wrote:
>>
>> > My own thought is that there's room for at least a few days' slop in
>> > the end date of the final commitfest, depending on what patches remain
>> > open and what the prospects are for getting them done.  (In the past
>> > we've sometimes let the final fest stretch on indefinitely, which is
>> > clearly the Wrong Thing; but that doesn't mean that the Right Thing is
>> > to say that it ends at 2017-04-01 00:00 UTC no matter what.)  The RMT
>> > should look at things in another day or two and make a judgment call
>> > about that.
>>
>> I was rather surprised to see the March commitfest declared to exactly
>> one month and feature freeze immediately thereafter.  Last time around
>> we left 2 weeks between CF end and feature freeze; the previous one I
>> think we had the final CF last two months.  Not stretch on indefinitely,
>> but we know the final CF for a cycle takes more effort than previous
>> ones, so it seems reasonable to give more time.  We have a large number
>> of patches still waiting for review.
>
> +1

+1


-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] PG_GETARG_GISTENTRY?

[Andres Freund]

Hi,

we have a good number of '(GISTENTRY *) PG_GETARG_POINTER(n)' in our
code - looks a bit better & shorter to have PG_GETARG_GISTENTRY(n).

Arugments against?

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump truncating queries in error messages

[Peter Eisentraut]

On 3/26/17 16:09, Tom Lane wrote:
> Peter Eisentraut  writes:
>> When reporting an error from a query, pg_dump truncates the reported
>> query to 128 characters (pg_backup_db.c ExecuteSqlCommand()).
> 
>> Is this (still) sensible?  The kind of queries that pg_dump is running
>> nowadays, I find myself unable to debug them if they are truncated at
>> that length.
> 
> Not clear that it ever was very sensible ... +1 for removing the limit.

OK done.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Schedule and Release Management Team for PG10

[Andres Freund]

On 2017-03-29 16:04:50 -0300, Alvaro Herrera wrote:
> Tom Lane wrote:
>
> > My own thought is that there's room for at least a few days' slop in
> > the end date of the final commitfest, depending on what patches remain
> > open and what the prospects are for getting them done.  (In the past
> > we've sometimes let the final fest stretch on indefinitely, which is
> > clearly the Wrong Thing; but that doesn't mean that the Right Thing is
> > to say that it ends at 2017-04-01 00:00 UTC no matter what.)  The RMT
> > should look at things in another day or two and make a judgment call
> > about that.
>
> I was rather surprised to see the March commitfest declared to exactly
> one month and feature freeze immediately thereafter.  Last time around
> we left 2 weeks between CF end and feature freeze; the previous one I
> think we had the final CF last two months.  Not stretch on indefinitely,
> but we know the final CF for a cycle takes more effort than previous
> ones, so it seems reasonable to give more time.  We have a large number
> of patches still waiting for review.

+1


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Schedule and Release Management Team for PG10

[Robert Haas]

On Wed, Mar 29, 2017 at 3:04 PM, Alvaro Herrera
 wrote:
> Tom Lane wrote:
>> My own thought is that there's room for at least a few days' slop in
>> the end date of the final commitfest, depending on what patches remain
>> open and what the prospects are for getting them done.  (In the past
>> we've sometimes let the final fest stretch on indefinitely, which is
>> clearly the Wrong Thing; but that doesn't mean that the Right Thing is
>> to say that it ends at 2017-04-01 00:00 UTC no matter what.)  The RMT
>> should look at things in another day or two and make a judgment call
>> about that.
>
> I was rather surprised to see the March commitfest declared to exactly
> one month and feature freeze immediately thereafter.  Last time around
> we left 2 weeks between CF end and feature freeze; the previous one I
> think we had the final CF last two months.  Not stretch on indefinitely,
> but we know the final CF for a cycle takes more effort than previous
> ones, so it seems reasonable to give more time.  We have a large number
> of patches still waiting for review.

That's true, but at
https://wiki.postgresql.org/wiki/PgCon_2016_Developer_Meeting we
agreed on March 31st as the feature freeze date.  I don't think the
RMT should substantially vary a date that was agreed by a developer
meeting with wide attendance; that seems like the tyranny of the few
over the many.  To quote from that page:

===
The dates agreed were:

CF1: September 1 to 30th
CF2: November 1 to 30th
CF3: January 1 to 31st.
CF4: March 1 to 31st.
Feature Freeze: March 31st
May 16th: Beta
===

I think we should be trying to hit those dates, not re-litigate them.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Reduce src/test/recovery verbosity

[Stephen Frost]

* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> Stephen Frost wrote:
> > * Dagfinn Ilmari Mannsåker (ilm...@ilmari.org) wrote:
> 
> > > Non-verbose prove still lists each test script, it just doesn't list
> > > each individual test within the script.
> > 
> > I agree that it'd be better to just show the per-script results rather
> > than every little test result because it's just too much.
> 
> Well, for some test files a single test could take half a minute.  A
> dozen lines taking in total about a minute seems fine to me.  The
> problem I see is a (pg_dump I think) script that has over 500 tests.

Not sure that I see that as a "problem". :)

> > If there's a way to change the verbosity for just those scripts, I'd be
> > happy to do that, if we're unable to agree on reducing it across the
> > board..
> 
> I'd rather silence only scripts that are overly verbose.

I'm fine with that, but how?

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] error handling in RegisterBackgroundWorker

[Robert Haas]

On Wed, Mar 29, 2017 at 2:10 PM, Peter Eisentraut
 wrote:
> How specifically would we do that?  And what user would choose the
> behavior "start this background worker but don't worry if it doesn't work"?

Well, if the background worker is auto-prewarm, you'd probably rather
have the database start rather than get unhappy about auto-prewarm
failing.  If the background worker is your logical replication
launcher it's a bit more serious, but if you have no subscriptions or
they're not that critical, maybe you don't care.  If the background
worker is in charge of telling your failover solution that this node
is up, then starting without it is entirely pointless.

I would be inclined to leave this alone for now and revisit it for a
future release.  I don't feel confident that we really know what the
right thing to do is here.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Schedule and Release Management Team for PG10

[Alvaro Herrera]

Tom Lane wrote:

> My own thought is that there's room for at least a few days' slop in
> the end date of the final commitfest, depending on what patches remain
> open and what the prospects are for getting them done.  (In the past
> we've sometimes let the final fest stretch on indefinitely, which is
> clearly the Wrong Thing; but that doesn't mean that the Right Thing is
> to say that it ends at 2017-04-01 00:00 UTC no matter what.)  The RMT
> should look at things in another day or two and make a judgment call
> about that.

I was rather surprised to see the March commitfest declared to exactly
one month and feature freeze immediately thereafter.  Last time around
we left 2 weeks between CF end and feature freeze; the previous one I
think we had the final CF last two months.  Not stretch on indefinitely,
but we know the final CF for a cycle takes more effort than previous
ones, so it seems reasonable to give more time.  We have a large number
of patches still waiting for review.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] TPC-H Q20 from 1 hour to 19 hours!

[Robert Haas]

On Mon, Mar 6, 2017 at 1:22 AM, Rafia Sabih
 wrote:
> This is to bring to notice a peculiar instance I found recently while
> running TPC-H benchmark queries. Q20 of the benchmark took 19 hours to
> complete ...

That's bad.

> It is clear that selectivity estimations are really bad in this case
> particularly at node,
>   ->  Merge Join  (cost=52959586.72..60024468.82 rows=85 width=16)
> (actual time=1525322.753..2419045.641 rows=1696742 loops=1)
>Merge Cond: ((lineitem.l_partkey =
> partsupp.ps_partkey) AND (lineitem.l_suppkey = partsupp.ps_suppkey))
>Join Filter:
> ((partsupp.ps_availqty)::numeric > ((0.5 * sum(lineitem.l_quantity
>Rows Removed by Join Filter: 3771

So, the selectivity estimation here is bad both before and after Tom's
commit, but it's significantly worse after (actual value 1696742, old
estimate 3771, new estimate 85).

> Still this puzzled me as during earlier runs of this benchmark I never
> encountered such prolonged running times. On further investigation I
> found that on reverting the commit
> 7fa93eec4e0c9c3e801e3c51aa4bae3a38aaa218
> Author: Tom Lane 
> Date:   Sat Dec 17 15:28:54 2016 -0500
> Fix FK-based join selectivity estimation for semi/antijoins.

I don't think the problem originates at the Merge Join, though,
because the commit says that at is fixing semi and anti-join estimates
- this is a plain inner join, so in theory it shouldn't change.
However, it's a bit hard for me to piece through these plans, the
formatting kind of got messed up - things are wrapped.  Could you
possibly attach the plans as attachments?

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Reduce src/test/recovery verbosity

[Alvaro Herrera]

Stephen Frost wrote:
> * Dagfinn Ilmari Mannsåker (ilm...@ilmari.org) wrote:

> > Non-verbose prove still lists each test script, it just doesn't list
> > each individual test within the script.
> 
> I agree that it'd be better to just show the per-script results rather
> than every little test result because it's just too much.

Well, for some test files a single test could take half a minute.  A
dozen lines taking in total about a minute seems fine to me.  The
problem I see is a (pg_dump I think) script that has over 500 tests.

> If there's a way to change the verbosity for just those scripts, I'd be
> happy to do that, if we're unable to agree on reducing it across the
> board..

I'd rather silence only scripts that are overly verbose.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication access control patches

[Alvaro Herrera]

Peter Eisentraut wrote:
> On 3/15/17 21:54, Peter Eisentraut wrote:

> > 0004 Add subscription apply worker privilege checks
> > 0005 Add CREATE SUBSCRIPTION privilege on databases
> 
> It would be nice to reach a conclusion on these (the second one
> particularly), because otherwise we'll be stuck with only superusers
> being allowed to create subscriptions.

I note that the CREATE privilege on databases, which previously only
enabled schema creation, now also allows to create publications.  I
wonder what is different about subscriptions that we need a separate
CREATE SUBSCRIPTION privilege; could we allow the three things under the
same privilege type?  (I suspect not; why give logical replication
controls to users who in previous releases were only able to create
schemas?)  If not, does it make sense to have one privilege for both new
things, perhaps something like GRANT LOGICAL REPLICATION THINGIES?  If
not, maybe we should have three separate priv bits: GRANT CREATE for
schemas, GRANT CREATE PUBLICATION and GRANT CREATE SUBSCRIPTION?


So this CREATE SUBSCRIPTION priv actually gives you the power to cause
the system to open network connections to the outside world.  It's not
something you give freely to random strangers -- should be guarded
moderately tight, because it could be used as covert channel for data
leaking.  However, it's 1000x better than requiring superuser for
subscription creation, so +1 for the current approach.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Monitoring roles patch

[Stephen Frost]

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> On 3/28/17 12:19, Dave Page wrote:
> > On Tue, Mar 28, 2017 at 11:39 AM, Peter Eisentraut
> >  wrote:
> >> On 3/28/17 11:34, Dave Page wrote:
> >>> On Tue, Mar 28, 2017 at 11:31 AM, Peter Eisentraut
> >>>  wrote:
>  This patch touches the pg_buffercache and pg_freespacemap extensions,
>  but there appear to be some files missing.
> >>>
> >>> Are you looking at an old version? There was one where I forgot to add
> >>> some files, but that was fixed within an hour or so in a new version.
> >>
> >> What is the name and date or your latest patch?
> > 
> > pg_monitor_v4.diff, 23rd March.
> 
> That is the patch I was looking at, and it's the one that is missing files.

Dave's currently hacking on a new patch based on our discussion, so I'd
suggest waiting another hour or so anyway until he's done.

Might be a bit longer as he's trying to do it in a hallway at
PGConf.US...

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Monitoring roles patch

[Peter Eisentraut]

On 3/28/17 12:19, Dave Page wrote:
> On Tue, Mar 28, 2017 at 11:39 AM, Peter Eisentraut
>  wrote:
>> On 3/28/17 11:34, Dave Page wrote:
>>> On Tue, Mar 28, 2017 at 11:31 AM, Peter Eisentraut
>>>  wrote:
 This patch touches the pg_buffercache and pg_freespacemap extensions,
 but there appear to be some files missing.
>>>
>>> Are you looking at an old version? There was one where I forgot to add
>>> some files, but that was fixed within an hour or so in a new version.
>>
>> What is the name and date or your latest patch?
> 
> pg_monitor_v4.diff, 23rd March.

That is the patch I was looking at, and it's the one that is missing files.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Schedule and Release Management Team for PG10

[Stephen Frost]

Tom, all,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> My own thought is that there's room for at least a few days' slop in
> the end date of the final commitfest, depending on what patches remain
> open and what the prospects are for getting them done.  (In the past
> we've sometimes let the final fest stretch on indefinitely, which is
> clearly the Wrong Thing; but that doesn't mean that the Right Thing is
> to say that it ends at 2017-04-01 00:00 UTC no matter what.)  The RMT
> should look at things in another day or two and make a judgment call
> about that.

I tend to agree with this, though I am also of the opinion that it'd
be better to get a date out there earlier rather than later.  There's a
number of hackers at PGConf.US who are trying their best to review
patches and keep things moving while also giving talks and being
involved in the conference.  There's at least a couple of patches which
I can think of off-hand that I'd like to continue working through the
process to get them committed and only require another day or so of
effort, but finding that time in-between activities at the conference,
while possible, isn't ideal.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] [PATCH] Reduce src/test/recovery verbosity

[Stephen Frost]

* Dagfinn Ilmari Mannsåker (ilm...@ilmari.org) wrote:
> Peter Eisentraut  writes:
> 
> > On 3/28/17 23:42, Michael Paquier wrote:
> >> src/bin/pg_dump and src/test/modules/test_pgdump generate too much
> >> output. If we could get tests to only print the final result, like how
> >> many tests done and how many have passed, things would be much
> >> friendlier.
> >
> > There are options to change the verbosity of things.
> >
> > I don't think I would like changing the default verbosity.  At least the
> > precedent from the pg_regress type tests is that there should be some
> > action on the screen as things run.
> 
> Non-verbose prove still lists each test script, it just doesn't list
> each individual test within the script.

I agree that it'd be better to just show the per-script results rather
than every little test result because it's just too much.

If there's a way to change the verbosity for just those scripts, I'd be
happy to do that, if we're unable to agree on reducing it across the
board..

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Hello Tom,


If someone were to put together a TAP test suite that covered all that
and made for a meaningful improvement in psql's altogether-miserable
code coverage report[1], I would think that that would be a useful
expenditure of buildfarm time.


Ok, this is an interesting point.

What I'm objecting to is paying the overhead for such a suite in order 
to test just this one thing.


Well, it should start somewhere. Once something is running it is easier to 
add more tests.



think that that passes the bang-for-buck test; or in other words, this
isn't the place I would start if I were creating a TAP suite for psql.


Sure, I would not have started with that either.

Note that from this patch point of view, it is somehow logical to start 
testing a given feature when this very feature is being developed...


The summary is that we agree that psql test coverage is abysmal, but you 
do not want to bootstrap a better test infrastructure for this particular 
and rather special new feature. Ok.


Maybe Corey can submit another patch with the exit 3 test removed.

--
Fabien.


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Other formats in pset like markdown, rst, mediawiki

[Jan Michálek]

2017-03-27 19:41 GMT+02:00 Jan Michálek :

>
>
> 2017-03-23 17:26 GMT+01:00 Pierre Ducroquet :
>
>> The following review has been posted through the commitfest application:
>> make installcheck-world:  tested, passed
>> Implements feature:   tested, passed
>> Spec compliant:   tested, passed
>> Documentation:tested, passed
>>
>> Hi
>>
>> This is my first review (Magnus said in his presentation in PGDay Paris
>> that volunteers should just come and help, so here I am), so please notify
>> me for any mistake I do when using the review tools...
>>
>> The feature seems to work as expected, but I don't claim to be a markdown
>> and rst expert.
>> Some minor issues with the code itself :
>> - some indentation issues (documentation and code itself with mix between
>> space based and tab based indentation) and a few trailing spaces in code
>>
>
> corrected
>
>
>> - typographic issues in the documentation :
>>   - "The html, asciidoc, latex, latex-longtable, troff-ms, and markdown
>> and rst formats" ==> duplicated and
>>
>
> corrected
>
>>   - "Sets the output format to one of unaligned, aligned, wrapped, html,
>> asciidoc, latex (uses tabular), latex-longtable, rst, markdown, or
>> troff-ms." ==> extra comma at the end of the list
>> - the comment " dont add line after last row, because line is added after
>> every row" is misleading, it should warn that it's only for rst
>> - there is a block of commented out code left
>> - in the print_aligned_vertical function, there is a mix between
>> "cont->opt->format == PRINT_RST" and "format == _rst" and I don't see
>> any obvious reason for that
>>
> corrected
>
>> - the documentation doesn't mention (but ok, it's kind of obvious) that
>> the linestyle option will not work with rst and markdown
>>
>>
> In this patch are corrected (i hope, i had correct changes in vimrc)
> indentation issues. Plese, look at this if it is OK (i men indentats) and
> some minor errors. And it should work on current master (probably).
>

Added \x option form markdown
In markdown works multiline cels (newline replaced by )
regre tests passed

Jan



>
> Have nice day
>
> Jan
>
>
>> Thanks !
>>
>> The new status of this patch is: Waiting on Author
>>
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>>
>
>
>
> --
> Jelen
> Starší čeledín datovýho chlíva
>



-- 
Jelen
Starší čeledín datovýho chlíva
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 2a9c412020..ee0c8fbea6 100644
*** a/doc/src/sgml/ref/psql-ref.sgml
--- b/doc/src/sgml/ref/psql-ref.sgml
***
*** 2366,2372  lo_import 152801
aligned, wrapped,
html, asciidoc,
latex (uses tabular),
!   latex-longtable, or
troff-ms.
Unique abbreviations are allowed.  (That would mean one letter
is enough.)
--- 2366,2373 
aligned, wrapped,
html, asciidoc,
latex (uses tabular),
! 		  latex-longtable, 
! 		  rst, markdown, or
troff-ms.
Unique abbreviations are allowed.  (That would mean one letter
is enough.)
***
*** 2394,2400  lo_import 152801
  

The html, asciidoc, latex,
!   latex-longtable, and troff-ms
formats put out tables that are intended to
be included in documents using the respective mark-up
language. They are not complete documents! This might not be
--- 2395,2402 
  

The html, asciidoc, latex,
! 		  latex-longtable, troff-ms,
! 		  markdown and rst
formats put out tables that are intended to
be included in documents using the respective mark-up
language. They are not complete documents! This might not be
diff --git a/src/bin/psql/command.c bindex 4f4a0aa9bd..97f4f37923 100644
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
***
*** 2518,2523  _align2string(enum printFormat in)
--- 2518,2529 
  		case PRINT_TROFF_MS:
  			return "troff-ms";
  			break;
+ 		case PRINT_MARKDOWN:
+ 			return "markdown";
+ 			break;
+ 		case PRINT_RST:
+ 			return "rst";
+ 			break;
  	}
  	return "unknown";
  }
***
*** 2589,2597  do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
  			popt->topt.format = PRINT_LATEX_LONGTABLE;
  		else if (pg_strncasecmp("troff-ms", value, vallen) == 0)
  			popt->topt.format = PRINT_TROFF_MS;
  		else
  		{
! 			psql_error("\\pset: allowed formats are unaligned, aligned, wrapped, html, asciidoc, latex, latex-longtable, troff-ms\n");
  			return false;
  		}
  	}
--- 2595,2607 
  			popt->topt.format = PRINT_LATEX_LONGTABLE;
  		else if (pg_strncasecmp("troff-ms", value, vallen) == 0)
  			popt->topt.format = 

Re: [HACKERS] error handling in RegisterBackgroundWorker

[Peter Eisentraut]

On 3/24/17 02:33, Michael Paquier wrote:
> What if we just let the user choose what they want with a new switch
> in bgw_flags, but keep LOG the default? One behavior and the other
> look both sensible to me.

How specifically would we do that?  And what user would choose the
behavior "start this background worker but don't worry if it doesn't work"?

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication access control patches

[Peter Eisentraut]

On 3/15/17 21:54, Peter Eisentraut wrote:
> 0001 Refine rules for altering publication owner
> 0002 Change logical replication pg_hba.conf use

These two were committed.

> 0003 Add USAGE privilege for publications

I'm withdrawing this one for now, because of some issues that were
discussed in the thread.

> 0004 Add subscription apply worker privilege checks
> 0005 Add CREATE SUBSCRIPTION privilege on databases

It would be nice to reach a conclusion on these (the second one
particularly), because otherwise we'll be stuck with only superusers
being allowed to create subscriptions.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [sqlsmith] Unpinning error in parallel worker

[Andreas Seltenreich]

Thomas Munro writes:

> Based on feedback on another thread about how to make reviewers' and
> committers' jobs easier, here is a format-patch version with a short
> description as raw material for a commit message, in case that is
> helpful.

+1

It's quite convenient.  Otherwise I have to be creative myself writing
commit messages in order to keep track of which patches are applied on
the branch sqlsmith is crunching on.

regards,
Andreas


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] sequence data type

[Peter Eisentraut]

Over at

is is being discussed that maybe the behavior when altering the sequence
type isn't so great, because it currently doesn't update the min/max
values of the sequence at all.  So here is a patch to update the min/max
values when the old min/max values were the min/max values of the data type.

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


0001-Adjust-min-max-values-when-changing-sequence-type.patch
Description: invalid/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Generic type subscripting

[Arthur Zakirov]

On 28.03.2017 19:31, Dmitry Dolgov wrote:

On 28 March 2017 at 12:08, Dmitry Dolgov <9erthali...@gmail.com
> wrote:


Wow, I didn't notice that, sorry - will fix it shortly.


So, here is the corrected version of the patch.


I have some picky comments.

I'm not sure that "typsbsparse" is better than "typsubscripting" or 
"typsubparse". Maybe "typsubsparse"?



  
+  typsubscripting
+  regproc


Here you didn't fix "typsubscripting" to new name.


+  JSON subscripting
+  
+   JSONB data type support array-style subscripting expressions to extract or 
update particular element. An example of subscripting syntax:


Should be "JSONB data type supports".


+  to handle subscripting expressions. It should contains logic for verification
+  and decide which function must be used for evaluation of this expression.
+  For instance:


Should be "It should contain".


+ 
+  JSON subscripting
+  
+   JSONB data type support array-style subscripting expressions to extract or 
update particular element. An example of subscripting syntax:


You have implemented jsonb subscripting. The documentation should be 
fixed to:


+ 
+  jsonb Subscripting
+  
+   jsonb data type support array-style subscripting 
expressions to extract or update particular element. An example of 
subscripting syntax:


I think IsOneOf() macros should be removed. Since it is not used anywhere.


+   Assert(subexpr != NULL);
+
+   if (subexpr == NULL)
+   ereport(ERROR,
+   (errcode(ERRCODE_DATATYPE_MISMATCH),
+errmsg("jsonb subscript does not support 
slices"),
+parser_errposition(pstate, 
exprLocation(
+   ((Node *) 
lfirst(sbsref->refupperindexpr->head));


Here one of the conditions is excess. Better to delete assert condition 
I think.


I've tried tests from message [1]. They looks good. Performance looks 
similar for subscripting without patch and with patch.


I wanted to implement subscripting for ltree or hstore extensions. 
Subscripting for ltree looks more interesting. Especially with slicing. 
But I haven't done it yet. I hope that I will implement it tomorrow.


1. 
https://www.postgresql.org/message-id/CAKNkYnz_WWkzzxyFx934N%3DEp47CAFju-Rk-sGeZo0ui8QdrGmw%40mail.gmail.com


--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Write Amplification Reduction Method (WARM)

[Alvaro Herrera]

Pavan Deolasee wrote:
> On Wed, Mar 29, 2017 at 3:42 AM, Alvaro Herrera 
> wrote:
> 
> > I pushed 0002 after some makeup, since it's just cosmetic and not
> > controversial.
> 
> Thanks. I think your patch of tracking interesting attributes seems ok too
> after the performance issue was addressed. Even though we can still improve
> that further, at least Mithun confirmed that there is no significant
> regression anymore and in fact for one artificial case, patch does better
> than even master.

Great, thanks.  I pushed it, too.  One optimization we could try is
using slot deform instead of repeated heap_getattr().  Patch is
attached.  I haven't benchmarked it.

On top of that, but perhaps getting in the realm of excessive
complication, we could see if the bitmapset is a singleton, and if it is
then do heap_getattr without creating the slot.  That'd require to have
a second copy of heap_tuple_attr_equals() that takes a HeapTuple instead
of a TupleTableSlot.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 0c3e2b0..976de99 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -56,6 +56,7 @@
 #include "access/xlogutils.h"
 #include "catalog/catalog.h"
 #include "catalog/namespace.h"
+#include "executor/tuptable.h"
 #include "miscadmin.h"
 #include "pgstat.h"
 #include "storage/bufmgr.h"
@@ -4337,7 +4338,7 @@ l2:
  */
 static bool
 heap_tuple_attr_equals(TupleDesc tupdesc, int attrnum,
-  HeapTuple tup1, HeapTuple tup2)
+  TupleTableSlot *tup1, TupleTableSlot 
*tup2)
 {
Datum   value1,
value2;
@@ -4366,13 +4367,10 @@ heap_tuple_attr_equals(TupleDesc tupdesc, int attrnum,
}
 
/*
-* Extract the corresponding values.  XXX this is pretty inefficient if
-* there are many indexed columns.  Should HeapDetermineModifiedColumns 
do
-* a single heap_deform_tuple call on each tuple, instead?  But that
-* doesn't work for system columns ...
+* Extract the corresponding values.
 */
-   value1 = heap_getattr(tup1, attrnum, tupdesc, );
-   value2 = heap_getattr(tup2, attrnum, tupdesc, );
+   value1 = slot_getattr(tup1, attrnum, );
+   value2 = slot_getattr(tup2, attrnum, );
 
/*
 * If one value is NULL and other is not, then they are certainly not
@@ -4424,17 +4422,27 @@ HeapDetermineModifiedColumns(Relation relation, 
Bitmapset *interesting_cols,
 {
int attnum;
Bitmapset *modified = NULL;
+   TupleTableSlot *oldslot;
+   TupleTableSlot *newslot;
+
+   oldslot = MakeSingleTupleTableSlot(RelationGetDescr(relation));
+   ExecStoreTuple(oldtup, oldslot, InvalidBuffer, false);
+   newslot = MakeSingleTupleTableSlot(RelationGetDescr(relation));
+   ExecStoreTuple(newtup, newslot, InvalidBuffer, false);
 
while ((attnum = bms_first_member(interesting_cols)) >= 0)
{
attnum += FirstLowInvalidHeapAttributeNumber;
 
if (!heap_tuple_attr_equals(RelationGetDescr(relation),
-  attnum, 
oldtup, newtup))
+   attnum, 
oldslot, newslot))
modified = bms_add_member(modified,
  
attnum - FirstLowInvalidHeapAttributeNumber);
}
 
+   ExecDropSingleTupleTableSlot(oldslot);
+   ExecDropSingleTupleTableSlot(newslot);
+
return modified;
 }
 

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] few fts functions for jsonb

[Andrew Dunstan]

On 26 March 2017 at 17:57, Dmitry Dolgov <9erthali...@gmail.com> wrote:
>> I'm not through looking at this. However, here are a few preliminary
>> comments
>
> I've attached new versions of the patches with improvements related to these
> commentaries.

These patches seem fundamentally OK. But I'm still not happy with the
naming etc.

I think the header changes would probably be better placed in
jsonapi.h or in a new header file.

And the names still seem too general to me. e.g. transform_json_values
should probably be transform_json_string_values, and the static
support functions should be renamed to match. Also the
JsonIterateAction and JsonTransformAction funtion typedefs should
probably be renamed to match.

I'm not sure there is any great point in the is_jsonb_data macro,
which is only used in one spot. I would get rid of it and expand the
test in place.

I don't have much time this week to work on it, as there are one or
two other patches I also want to look at.  If you clean these things
up I will commit it. The second patch looks fine.

cheers

andrew

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical decoding of two-phase transactions

[Stas Kelvich]

> On 28 Mar 2017, at 18:08, Andres Freund  wrote:
> 
> On 2017-03-28 15:55:15 +0100, Simon Riggs wrote:
>> 
>> 
>> That assertion is obviously false... the plugin can resolve this in
>> various ways, if we allow it.
> 
> Handling it by breaking replication isn't handling it (e.g. timeouts in
> decoding etc).  Handling it by rolling back *prepared* transactions
> (which are supposed to be guaranteed to succeed!), isn't either.
> 
> 
>> You can say that in your opinion you prefer to see this handled in
>> some higher level way, though it would be good to hear why and how.
> 
> It's pretty obvious why: A bit of DDL by the user shouldn't lead to the
> issues mentioned above.
> 
> 
>> Bottom line here is we shouldn't reject this patch on this point,
> 
> I think it definitely has to be rejected because of that.  And I didn't
> bring this up at the last minute, I repeatedly brought it up before.
> Both to Craig and Stas.

Okay. In order to find more realistic cases that blocks replication
i’ve created following setup:

* in backend: tests_decoding plugins hooks on xact events and utility
statement hooks and transform each commit into prepare, then sleeps
on latch. If transaction contains DDL that whole statement pushed in
wal as transactional message. If DDL can not be prepared or disallows
execution in transaction block than it goes as nontransactional logical
message without prepare/decode injection. If transaction didn’t issued any
DDL and didn’t write anything to wal, then it skips 2pc too.

* after prepare is decoded, output plugin in walsender unlocks backend
allowing to proceed with commit prepared. So in case when decoding
tries to access blocked catalog everything should stop.

* small python script that consumes decoded wal from walsender (thanks
Craig and Petr)

After small acrobatics with that hooks I’ve managed to run whole
regression suite in parallel mode through such setup of test_decoding
without any deadlocks. I’ve added two xact_events to postgres and
allowedn prepare of transactions that touched temp tables since
they are heavily used in tests and creates a lot of noise in diffs.

So it boils down to 3 failed regression tests out of 177, namely:

* transactions.sql — here commit of tx stucks with obtaining SafeSnapshot().
I didn’t look what is happening there specifically, but just checked that
walsender isn’t blocked. I’m going to look more closely at this.

* prepared_xacts.sql — here select prepared_xacts() sees our prepared
tx. It is possible to filter them out, but obviously it works as expected.

* guc.sql — here pendingActions arrives on 'DISCARD ALL’ preventing tx
from being prepared. I didn’t found the way to check presence of
pendingActions outside of async.c so decided to leave it as is.

It seems that at least in regression tests nothing can block twophase
logical decoding. Is that strong enough argument to hypothesis that current
approach doesn’t creates deadlock except locks on catalog which should be
disallowed anyway?

Patches attached. logical_twophase_v5 is slightly modified version of previous
patch merged with Craig’s changes. Second file is set of patches over previous
one, that implements logic i’ve just described. There is runtest.sh script that
setups postgres, runs python logical consumer in background and starts
regression test.


Stas Kelvich
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company



logical_twophase_v5.diff
Description: Binary data


logical_twophase_regresstest.diff
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Logical decoding on standby

[Simon Riggs]

On 29 March 2017 at 10:17, Craig Ringer  wrote:
> On 29 March 2017 at 16:44, Craig Ringer  wrote:
>
>> * Split oldestCatalogXmin tracking into separate patch
>
> Regarding this, Simon raised concerns about xlog volume here.
>
> It's pretty negligible.
>
> We only write a new record when a vacuum runs after catalog_xmin
> advances on the slot with the currently-lowest catalog_xmin (or, if
> vacuum doesn't run reasonably soon, when the bgworker next looks).

I'd prefer to slow things down a little, not be so eager.

If we hold back update of the catalog_xmin until when we run
GetRunningTransactionData() we wouldn't need to produce any WAL
records at all AND we wouldn't need to have VACUUM do
UpdateOldestCatalogXmin(). Bgwriter wouldn't need to perform an extra
task.

That would also make this patch about half the length it is.

Let me know what you think.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Multiple TO version in ALTER EXTENSION UPDATE

[Daniel Gustafsson]

While reading I noticed that we allow multiple TO  in ALTER EXTENSION
UPDATE, and defer throwing a syntax error until command processing.  Is there a
reason for deferring and not handling it in gram.y directly as in the attached
patch since it is in fact a syntax error?  It yields a different error message
to the user, but makes for easier to read code (IMH-and-biased-O).

cheers ./daniel



extension_update_syntax.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: pageinspect / add page_checksum and bt_page_items(bytea)

[Tomas Vondra]

On 03/24/2017 04:27 AM, Peter Eisentraut wrote:

On 3/17/17 18:35, Tomas Vondra wrote:

On 03/17/2017 05:23 PM, Peter Eisentraut wrote:

I'm struggling to find a good way to share code between
bt_page_items(text, int4) and bt_page_items(bytea).

If we do it via the SQL route, as I had suggested, it makes the
extension non-relocatable, and it will also create a bit of a mess
during upgrades.

If doing it in C, it will be a bit tricky to pass the SRF context
around.  There is no "DirectFunctionCall within SRF context", AFAICT.


Not sure what it has to do with DirectFunctionCall? You want to call the
bytea variant from the existing one? Wouldn't it be easier to simply
define a static function with the shared parts, and pass around the
fctx/fcinfo? Not quite pretty, but should work.


Perhaps what was added in

would actually work here.



I've tried to refactor the code using this, but the result was rather 
ugly because (a) it really is quite tricky to pass around the contexts 
and (b) the sanity checks are quite different for the two input types, 
so mixing those parts (essentially the SRF_IS_FIRSTCALL bits) does not 
make much sense IMHO.


The attached patch is the best I came up with - it essentially shares 
just the tuple-forming part, which is exactly the same in both cases.


It also adds the P_ISMETA(opaque) check to the original function, which 
seems like a useful defense against page written to a different place 
(which is essentially the issue I was originally investigating).


regards

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [COMMITTERS] pgsql: Improve access to parallel query from procedural languages.

[Robert Haas]

On Wed, Mar 29, 2017 at 12:02 AM, Rafia Sabih
 wrote:
> On Tue, Mar 28, 2017 at 9:05 PM, Robert Haas  wrote:
>> OK, but don't pg_event_trigger_dropped_objects and
>> pg_event_trigger_ddl_commands need the same treatment?
>>
> Done.
> I was only concentrating on the build farm failure cases, otherwise I
> think more work might be required in this direction.

Sure, but there's some happy medium between checking every line in
pg_proc.h for an error and checking nothing other than the functions
immediately.  In this case, there's a group of 4 similarly-named
functions with a similar problem and you changed only the middle two.
Trying to audit the entire file for other mistakes is probably a
fruitless response to this discovery, but auditing the other functions
defined in the same file and with the same naming pattern as the one
you changed seems like something you should do.

Anyway, thanks for debugging this.  Committed this version.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Monitoring roles patch

[Stephen Frost]

Dave,

* Dave Page (dp...@pgadmin.org) wrote:
> OK, so essentially what I did, except s/pg_read_all_stats/pg_read_all_queries 
> ?

Yup.

> So pgstattuple, pg_sfreespacemap, pg_visibility and pgrowlocks to be
> allowed access from members of pg_stat_scan_tables, which in turn is
> granted to pg_monitor?

Yes.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Partitioned tables and relfilenode

[Robert Haas]

On Wed, Mar 29, 2017 at 4:21 AM, Amit Langote
 wrote:
> Looks correct, so incorporated in the attached updated patch.  Thanks.

This seems like a hacky way to limit the reloptions to just OIDs.
Shouldn't we instead have a new RELOPT_KIND_PARTITIONED or something
like that?

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [sqlsmith] Unpinning error in parallel worker

[Robert Haas]

On Wed, Mar 29, 2017 at 1:31 AM, Thomas Munro
 wrote:
> I considered whether the error message could be improved but it
> matches the message for an existing similar case (where you try to
> attach to an unknown handle).

Ugh, OK.  I committed this, but I think this whole file needs a visit
from the message style police.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Reduce src/test/recovery verbosity

[Dagfinn Ilmari Mannsåker]

Peter Eisentraut  writes:

> On 3/28/17 23:42, Michael Paquier wrote:
>> src/bin/pg_dump and src/test/modules/test_pgdump generate too much
>> output. If we could get tests to only print the final result, like how
>> many tests done and how many have passed, things would be much
>> friendlier.
>
> There are options to change the verbosity of things.
>
> I don't think I would like changing the default verbosity.  At least the
> precedent from the pg_regress type tests is that there should be some
> action on the screen as things run.

Non-verbose prove still lists each test script, it just doesn't list
each individual test within the script.

-- 
"A disappointingly low fraction of the human race is,
 at any given time, on fire." - Stig Sandbeck Mathisen


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Tom Lane]

Fabien COELHO  writes:
>> If we're sufficiently dead set on it, we could go back to the TAP-based 
>> approach,

> Hmmm. You rejected it. I agree that TAP tests are not well suited for some 
> simple tests because of their initdb overhead.

>> but I still doubt that this test is worth the amount of overhead that 
>> would add.

> I think that there is an underlying issue with keeping on rejecting tests 
> which aim at having a reasonable code coverage of features by exercising 
> different paths.

There's certainly a fair amount of psql behavior that's not adequately
testable within the standard regression test infrastructure.  Parsing of
command line arguments and exit codes for unusual cases both fall into
that area, and then there's things like prompts and tab completion.
If someone were to put together a TAP test suite that covered all that
and made for a meaningful improvement in psql's altogether-miserable
code coverage report[1], I would think that that would be a useful
expenditure of buildfarm time.  What I'm objecting to is paying the
overhead for such a suite in order to test just this one thing.  I don't
think that that passes the bang-for-buck test; or in other words, this
isn't the place I would start if I were creating a TAP suite for psql.

regards, tom lane

[1] https://coverage.postgresql.org/src/bin/psql/index.html


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Reduce src/test/recovery verbosity

[Peter Eisentraut]

On 3/28/17 23:42, Michael Paquier wrote:
> src/bin/pg_dump and src/test/modules/test_pgdump generate too much
> output. If we could get tests to only print the final result, like how
> many tests done and how many have passed, things would be much
> friendlier.

There are options to change the verbosity of things.

I don't think I would like changing the default verbosity.  At least the
precedent from the pg_regress type tests is that there should be some
action on the screen as things run.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Allow interrupts on waiting standby

[Michael Paquier]

On Wed, Mar 29, 2017 at 5:04 PM, Tsunakawa, Takayuki
 wrote:
> From: pgsql-hackers-ow...@postgresql.org
>> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
>> What do you think about the updated version attached?
> I reviewed this patch.  Here are some comments and questions:

Thanks!

> (1) monitoring.sgml
> The new row needs to be placed in the "Timeout" group.  The current patch 
> puts the row at the end of IO group.  Please insert the new row according to 
> the alphabetical order.
>
> In addition, "morerows" attribute of the following line appears to be 
> increased.
>
>  Timeout
>
> By the way, doesn't this wait event belong to IPC wait event type, because 
> the process is waiting for other conflicting processes to terminate the 
> conflict conditions?  Did you choose Timeout type because 
> max_standby_{archive | streaming}_delay relates to this wait?  I'm not 
> confident which is appropriate, but I'm afraid users can associate this wait 
> with a timeout.

Actually I think that this event belongs to the timeout category,
because we wait until the timeout has finished, the latch being here
to be sure that the process is more responsive in case of a postmaster
death.

> (2) standby.c
> Do we need to specify WL_LATCH_SET?  Who can set the latch?  Do the backends 
> who ended the conflict set the latch?

This makes the process able to react on SIGHUP. That's useful for
responsiveness.

> (3) standby.c
> +   if (rc & WL_LATCH_SET)
> +   ResetLatch(MyLatch);
> +
> +   /* emergency bailout if postmaster has died */
> +   if (rc & WL_POSTMASTER_DEATH)
> +   proc_exit(1);
>
> I thought the child processes have to terminate as soon as postmaster 
> vanishes.  So, it would be better for the order of the two if statements 
> above to be reversed.  proc_exit() could be exit(), as some children do in 
> postmaster/*.c.

OK, reversed this order.

Attached is v4.
-- 
Michael


standby-delay-latch-v4.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Logical replication existing data copy

[Petr Jelinek]

On 29/03/17 10:14, Erik Rijkers wrote:
> On 2017-03-09 11:06, Erik Rijkers wrote:

 I use three different machines (2 desktop, 1 server) to test logical
 replication, and all three have now at least once failed to correctly
 synchronise a pgbench session (amidst many succesful runs, of course)
>>>
> 
> 
> (At the moment using tese patches for tests:)
> 
>> 0001-Reserve-global-xmin-for-create-slot-snasphot-export.patch +
>> 0002-Don-t-use-on-disk-snapshots-for-snapshot-export-in-l.patch+
>> 0003-Prevent-snapshot-builder-xmin-from-going-backwards.patch  +
>> 0004-Fix-xl_running_xacts-usage-in-snapshot-builder.patch  +
>> 0005-Skip-unnecessary-snapshot-builds.patch+
> 
> 
> The failed tests that I kept seeing (see the
> pgbench-over-logical-replication tests upthread) were never really
> 'solved'.
> 
> 
> But I have now finally figured out what caused these unexpected failed
> tests: it was  wal_sender_timeout  or rather, its default of 60 s.
> 
> This caused 'terminating walsender process due to replication timeout'
> on the primary (not strictly an error), and the concomittant ERROR on
> the replica: 'could not receive data from WAL stream: server closed the
> connection unexpectedly'.
> 
> here is a typical example (primary/replica logs time-intertwined, with
> 'primary'):
> 
> [...]
> 2017-03-24 16:21:38.129 CET [15002]  primaryLOG:  using stale
> statistics instead of current ones because stats collector is not
> responding
> 2017-03-24 16:21:42.690 CET [27515]  primaryLOG:  using stale
> statistics instead of current ones because stats collector is not
> responding
> 2017-03-24 16:21:42.965 CET [14999]replica  LOG:  using stale
> statistics instead of current ones because stats collector is not
> responding
> 2017-03-24 16:21:49.816 CET [14930]  primaryLOG:  terminating
> walsender process due to
> 2017-03-24 16:21:49.817 CET [14926]replica  ERROR:  could not
> receive data from WAL stream: server closed the connection unexpectedly
> 2017-03-24 16:21:49.824 CET [27502]replica  LOG:  worker process:
> logical replication worker for subscription 24864 (PID 14926) exited
> with exit code 1
> 2017-03-24 16:21:49.824 CET [27521]replica  LOG:  starting logical
> replication worker for subscription "sub1"
> 2017-03-24 16:21:49.828 CET [15008]replica  LOG:  logical
> replication apply for subscription sub1 started
> 2017-03-24 16:21:49.832 CET [15009]  primaryLOG:  received
> replication command: IDENTIFY_SYSTEM
> 2017-03-24 16:21:49.832 CET [15009]  primaryLOG:  received
> replication command: START_REPLICATION SLOT "sub1" LOGICAL 3/FC976440
> (proto_version '1', publication_names '"pub1"')
> 2017-03-24 16:21:49.833 CET [15009]  primaryDETAIL:  streaming
> transactions committing after 3/FC889810, reading WAL from 3/FC820FC0
> 2017-03-24 16:21:49.833 CET [15009]  primaryLOG:  starting logical
> decoding for slot "sub1"
> 2017-03-24 16:21:50.471 CET [15009]  primaryDETAIL:  Logical
> decoding will begin using saved snapshot.
> 2017-03-24 16:21:50.471 CET [15009]  primaryLOG:  logical decoding
> found consistent point at 3/FC820FC0
> 2017-03-24 16:21:51.169 CET [15008]replica  DETAIL:  Key
> (hid)=(9014) already exists.
> 2017-03-24 16:21:51.169 CET [15008]replica  ERROR:  duplicate key
> value violates unique constraint "pgbench_history_pkey"
> 2017-03-24 16:21:51.170 CET [27502]replica  LOG:  worker process:
> logical replication worker for subscription 24864 (PID 15008) exited
> with exit code 1
> 2017-03-24 16:21:51.170 CET [27521]replica  LOG:  starting logical
> replication worker for subscription "sub1"
> [...]
> 
> My primary and replica were always on a single machine (making it more
> likely that that timeout is reached?).  In my testing it seems that
> reaching the timeout on the primary (and 'closing the connection
> unexpectedly' on the replica) does not necessarily break the logical
> replication.  But almost all log-rep failures that I have seen were
> started by this sequence of events.
> 
> After setting  wal_sender_timeout  to 3 minutes there were no more
> failed tests.
> 
> Perhaps it warrants setting  wal_sender_timeout  a bit higher than the
> current default of 60 seconds?  After all I also saw the 'replication
> timeout' / 'closed the connection' couple rather often during
> not-failing tests.  (These also disappeared, almost completely,  with a
> higher  setting of wal_sender_timeout)
> 
> In any case it would be good to mention the setting (and its potentially
> deteriorating effect) somehere nearer the logical replication treatment.
> 
> ( I read about wal_sender_timeout and keepalive ping, perhaps there's
> (still) something amiss there? Just a guess, I don't know )
> 
> As I said, I saw no more failures with the higher 3 minute setting, with
> one exception: the one test that straddled the DST change (saterday 24
> march 02:00 h).  I am happy to discount that one failure 

Re: [HACKERS] Adding support for Default partition in partitioning

[Rahila Syed]

Thanks for reporting. I have identified the problem and have a fix.
Currently working on allowing
adding a partition after default partition if the default partition does
not have any conflicting rows.
Will update the patch with both of these.

Thank you,
Rahila Syed

On Mon, Mar 27, 2017 at 12:10 PM, Rushabh Lathia 
wrote:

> I applied the patch and was trying to perform some testing, but its
> ending up with server crash with the test shared by you in your starting
> mail:
>
> postgres=# CREATE TABLE list_partitioned (
> postgres(# a int
> postgres(# ) PARTITION BY LIST (a);
> CREATE TABLE
> postgres=#
> postgres=# CREATE TABLE part_default PARTITION OF list_partitioned FOR
> VALUES IN (DEFAULT);
> CREATE TABLE
>
> postgres=# CREATE TABLE part_1 PARTITION OF list_partitioned FOR VALUES IN
> (4,5);
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
>
> Apart from this, few more explanation in the patch is needed to explain the
> changes for the DEFAULT partition. Like I am not quite sure what exactly
> the
> latest version of patch supports, like does that support the tuple row
> movement,
> or adding new partition will be allowed having partition table having
> DEFAULT
> partition, which is quite difficult to understand through the code changes.
>
> Another part which is missing in the patch is the test coverage, adding
> proper test coverage, which explain what is supported and what's not.
>
> Thanks,
>
> On Fri, Mar 24, 2017 at 3:25 PM, Rahila Syed 
> wrote:
>
>> Hello Rushabh,
>>
>> Thank you for reviewing.
>> Have addressed all your comments in the attached patch. The attached
>> patch currently throws an
>> error if a new partition is added after default partition.
>>
>> >Rather then adding check for default here, I think this should be
>> handle inside
>> >get_qual_for_list().
>> Have moved the check inside get_qual_for_partbound() as needed to do some
>> operations
>> before calling get_qual_for_list() for default partitions.
>>
>> Thank you,
>> Rahila Syed
>>
>> On Tue, Mar 21, 2017 at 11:36 AM, Rushabh Lathia <
>> rushabh.lat...@gmail.com> wrote:
>>
>>> I picked this for review and noticed that patch is not getting
>>> cleanly complied on my environment.
>>>
>>> partition.c: In function ‘RelationBuildPartitionDesc’:
>>> partition.c:269:6: error: ISO C90 forbids mixed declarations and code
>>> [-Werror=declaration-after-statement]
>>>   Const*val = lfirst(c);
>>>   ^
>>> partition.c: In function ‘generate_partition_qual’:
>>> partition.c:1590:2: error: ISO C90 forbids mixed declarations and code
>>> [-Werror=declaration-after-statement]
>>>   PartitionBoundSpec *spec = (PartitionBoundSpec *)bound;
>>>   ^
>>> partition.c: In function ‘get_partition_for_tuple’:
>>> partition.c:1820:5: error: array subscript has type ‘char’
>>> [-Werror=char-subscripts]
>>>  result = parent->indexes[partdesc->boundinfo->def_index];
>>>  ^
>>> partition.c:1824:16: error: assignment makes pointer from integer
>>> without a cast [-Werror]
>>>  *failed_at = RelationGetRelid(parent->reldesc);
>>> ^
>>> cc1: all warnings being treated as errors
>>>
>>> Apart from this, I was reading patch here are few more comments:
>>>
>>> 1) Variable initializing happening at two place.
>>>
>>> @@ -166,6 +170,8 @@ RelationBuildPartitionDesc(Relation rel)
>>>  /* List partitioning specific */
>>>  PartitionListValue **all_values = NULL;
>>>  boolfound_null = false;
>>> +boolfound_def = false;
>>> +intdef_index = -1;
>>>  intnull_index = -1;
>>>
>>>  /* Range partitioning specific */
>>> @@ -239,6 +245,8 @@ RelationBuildPartitionDesc(Relation rel)
>>>  i = 0;
>>>  found_null = false;
>>>  null_index = -1;
>>> +found_def = false;
>>> +def_index = -1;
>>>  foreach(cell, boundspecs)
>>>  {
>>>  ListCell   *c;
>>> @@ -249,6 +257,15 @@ RelationBuildPartitionDesc(Relation rel)
>>>
>>>
>>> 2)
>>>
>>> @@ -1558,6 +1586,19 @@ generate_partition_qual(Relation rel)
>>>  bound = stringToNode(TextDatumGetCString(boundDatum));
>>>  ReleaseSysCache(tuple);
>>>
>>> +/* Return if it is a default list partition */
>>> +PartitionBoundSpec *spec = (PartitionBoundSpec *)bound;
>>> +ListCell *cell;
>>> +foreach(cell, spec->listdatums)
>>>
>>> More comment on above hunk is needed?
>>>
>>> Rather then adding check for default here, I think this should be handle
>>> inside
>>> get_qual_for_list().
>>>
>>> 3) Code is not aligned with existing
>>>
>>> diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
>>> index 6316688..ebb7db7 100644
>>> --- a/src/backend/parser/gram.y
>>> +++ b/src/backend/parser/gram.y

Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

[Andreas Karlsson]

On 03/29/2017 05:43 AM, Haribabu Kommi wrote:
> Updated patch attached.

I get a test failure in the pg_upgrade tests, but I do not have time 
right now to investigate.


The failing test is "Restoring database schemas in the new cluster".

I get the following in the log:

command: 
"/home/andreas/dev/postgresql/src/bin/pg_upgrade/tmp_check/install//home/andreas/dev/postgresql-inst/bin/pg_dump" 
--host /home/andreas/dev/postgresql/src/bin/pg_upgrade --port 50848 
--username andreas --schema-only --quote-all-identifiers 
--binary-upgrade --format=custom  --file="pg_upgrade_dump_16385.custom" 
'dbname='"'"'./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ'"'"'' >> 
"pg_upgrade_dump_16385.log" 2>&1



command: 
"/home/andreas/dev/postgresql/src/bin/pg_upgrade/tmp_check/install//home/andreas/dev/postgresql-inst/bin/pg_restore" 
--host /home/andreas/dev/postgresql/src/bin/pg_upgrade --port 50848 
--username andreas --exit-on-error --verbose --dbname 
'dbname='"'"'./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ'"'"'' 
"pg_upgrade_dump_16385.custom" >> "pg_upgrade_dump_16385.log" 2>&1

pg_restore: connecting to database for restore
pg_restore: [archiver (db)] connection to database 
"./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ" failed: FATAL:  database 
"./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ" does not exist


Andreas


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [POC] A better way to expand hash indexes.

[Mithun Cy]

Thanks, Amit for a detailed review.

On Wed, Mar 29, 2017 at 4:09 PM, Amit Kapila  wrote:
> Okay, your current patch looks good to me apart from minor comments,
> so marked as Read For Committer.  Please either merge the
> sort_hash_b_2.patch with main patch or submit it along with next
> revision for easier reference.

I will keep it separated just in case commiter likes
sortbuild_hash_A.patch. We can use either of sortbuild_hash_*.patch on
top of main patch.

>
> Few minor comments:
> 1.
> +splitpoint phase S.  The hashm_spares[0] is always 0, so that buckets 0 and 1
> +(which belong to splitpoint group 0's phase 1 and phase 2 respectively) 
> always
> +appear at block numbers 1 and 2, just after the meta page.
>
> This explanation doesn't seem to be right as with current patch we
> start phased allocation only after splitpoint group 9.

Again a mistake, removed the sentence in parentheses.

> 2.
> -#define HASH_MAX_SPLITPOINTS 32
>  #define HASH_MAX_BITMAPS 128
>
> +#define SPLITPOINT_PHASES_PER_GRP 4
> +#define SPLITPOINT_PHASE_MASK (SPLITPOINT_PHASES_PER_GRP - 1)
> +#define SPLITPOINT_GROUPS_WITH_ONLY_ONE_PHASE 10
> +#define HASH_MAX_SPLITPOINTS \
> + ((32 - SPLITPOINT_GROUPS_WITH_ONLY_ONE_PHASE) * \
> + SPLITPOINT_PHASES_PER_GRP) + \
> + SPLITPOINT_GROUPS_WITH_ONLY_ONE_PHASE
>
> You have changed the value of HASH_MAX_SPLITPOINTS, but the comments
> explaining that value are still unchanged.  Refer below text.
> "The limitation on the size of spares[] comes from the fact that there's
>  * no point in having more than 2^32 buckets with only uint32 hashcodes."

The limitation is still indirectly imposed by the fact that we can
have only 2^32 buckets. But I also added a note that
HASH_MAX_SPLITPOINTS also considers that after
SPLITPOINT_GROUPS_WITH_ONLY_ONE_PHASE bucket allocation will be done
in multiple(exactly 4) phases.

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com


yet_another_expand_hashbucket_efficiently_11.patch
Description: Binary data


sortbuild_hash_B_2.patch
Description: Binary data


sortbuild_hash_A.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [POC] A better way to expand hash indexes.

[Mithun Cy]

That means at every new
+split point we double the existing number of buckets.  Allocating huge chucks

On Mon, Mar 27, 2017 at 11:56 PM, Jesper Pedersen
 wrote:

> I ran some performance scenarios on the patch to see if the increased
> 'spares' allocation had an impact. I havn't found any regressions in that
> regard.

Thanks Jasper for testing the patch.

> Attached patch contains some small fixes, mainly to the documentation - on
> top of v7.

I have taken some of the grammatical and spell check issues you have
mentioned. One major thing I left it as it is term "splitpoint" which
you have tried to change in many places to "split point", The
splitpoint is not introduced by me, it was already used in many
places, so I think it is acceptable to use that term. I think I shall
not add changes which are not part of the core issue. I think another
patch on top of this should be okay.

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Write Amplification Reduction Method (WARM)

[Robert Haas]

On Wed, Mar 29, 2017 at 7:12 AM, Amit Kapila  wrote:
> No as I agreed above, it won't double-compress, but still looks
> slightly risky to rely on different set of values passed to
> index_form_tuple and then compare them.

It assumes that the compressor is completely deterministic, which I'm
fairly is true today, but might be false in the future.  For example:

https://groups.google.com/forum/#!topic/snappy-compression/W8v_ydnEPuc

We've talked about using snappy as a compression algorithm before, and
if the above post is correct, an upgrade to the snappy library version
is an example of a change that would break the assumption in question.
I think it's generally true for almost any modern compression
algorithm (including pglz) that there are multiple compressed texts
that would decompress to the same uncompressed text.  Any algorithm is
required to promise that it will always produce one of the compressed
texts that decompress back to the original, but not necessarily that
it will always produce the same one.

As another example of this, consider that zlib (gzip) has a variety of
options to control compression behavior, such as, most obviously, the
compression level (1 .. 9).

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: Write Amplification Reduction Method (WARM)

[Amit Kapila]

On Wed, Mar 29, 2017 at 1:10 PM, Pavan Deolasee
 wrote:
>
> On Wed, Mar 29, 2017 at 12:02 PM, Amit Kapila 
> wrote:
>>
>> On Wed, Mar 29, 2017 at 11:52 AM, Amit Kapila 
>> wrote:
>
> Then during recheck, we pass already compressed values to
> index_form_tuple(). But my point is, the following code will ensure that we
> don't compress it again. My reading is that the first check for
> !VARATT_IS_EXTENDED will return false if the value is already compressed.
>

You are right.  I was confused with previous check of VARATT_IS_EXTERNAL.

>
> TBH I couldn't find why the original index insertion code will always supply
> uncompressed values.
>

Just try by inserting large value of text column ('aa.bbb')
upto 2.5K.  Then have a breakpoint in heap_prepare_insert and
index_form_tuple, and debug both the functions, you can find out that
even though we compress during insertion in heap, the index will
compress the original value again.

> But even if does, and even if the recheck gets it in
> compressed form, I don't see how we will double-compress that.
>

No as I agreed above, it won't double-compress, but still looks
slightly risky to rely on different set of values passed to
index_form_tuple and then compare them.

> As far as, comparing two compressed values go, I don't see a problem there.
> Exact same compressed values should decompress to exact same value. So
> comparing two compressed values and two uncompressed values should give us
> the same result.
>

Yeah probably you are right, but I am not sure if it is good idea to
compare compressed values.

I think with this new changes in btrecheck, it would appear to be much
costlier as compare to what you have few versions back.  I am afraid
that it can impact performance for cases where there are few WARM
updates in chain and many HOT updates as it will run recheck for all
such updates.  Did we any time try to measure the performance of cases
like that?

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Use American English spelling in pg_waldump error message

[Daniel Gustafsson]

We use “unrecognize” rather than “unrecognise” in all other error messages in
the tree, the attached patch fixes the one place where the latter spelling was
used.

cheers ./daniel



pg_waldump_errmsg.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] gitlab post-mortem: pg_basebackup waiting for checkpoint

[Michael Banck]

Hi,

Am Montag, den 27.02.2017, 16:20 +0100 schrieb Magnus Hagander:
> On Sun, Feb 26, 2017 at 9:59 PM, Tom Lane  wrote:
> Is there an argument for back-patching this?
> 
> 
> Seems you were typing that at the same time as we did.
> 
> 
> I'm considering it, but not swayed in either direction. Should I take
> your comment as a vote that we should back-patch it? 

I've checked back into this thread, and there seems to be a +1 from Tom
and a +(0.5-1) from Simon for backpatching, and no obvious -1s. Did you
decide against it in the end, or is this still an open item?


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [POC] A better way to expand hash indexes.

[Amit Kapila]

On Wed, Mar 29, 2017 at 12:51 PM, Mithun Cy  wrote:
> On Wed, Mar 29, 2017 at 10:12 AM, Amit Kapila  wrote:
>> Few other comments:
>> +/*
>> + * This is just a trick to save a division operation. If you look into the
>> + * bitmap of 0-based bucket_num 2nd and 3rd most significant bit will 
>> indicate
>> + * which phase of allocation the bucket_num belongs to with in the group. 
>> This
>> + * is because at every splitpoint group we allocate (2 ^ x) buckets and we 
>> have
>> + * divided the allocation process into 4 equal phases. This macro returns 
>> value
>> + * from 0 to 3.
>> + */
>>
>> +#define SPLITPOINT_PHASES_WITHIN_GROUP(sp_g, bucket_num) \
>> + (((bucket_num) >> (sp_g - SPLITPOINT_GROUPS_WITH_ONLY_ONE_PHASE)) & \
>> + SPLITPOINT_PHASE_MASK)
>> This won't work if we change SPLITPOINT_GROUPS_WITH_ONLY_ONE_PHASE to
>> number other than 3.  I think you should change it so that it can work
>> with any value of  SPLITPOINT_GROUPS_WITH_ONLY_ONE_PHASE.
>
> Fixed, using SPLITPOINT_GROUPS_WITH_ONLY_ONE_PHASE was accidental. All
> I need is most significant 3 bits hence should be subtracted by 3
> always.
>

Okay, your current patch looks good to me apart from minor comments,
so marked as Read For Committer.  Please either merge the
sort_hash_b_2.patch with main patch or submit it along with next
revision for easier reference.

Few minor comments:
1.
+splitpoint phase S.  The hashm_spares[0] is always 0, so that buckets 0 and 1
+(which belong to splitpoint group 0's phase 1 and phase 2 respectively) always
+appear at block numbers 1 and 2, just after the meta page.

This explanation doesn't seem to be right as with current patch we
start phased allocation only after splitpoint group 9.

2.
-#define HASH_MAX_SPLITPOINTS 32
 #define HASH_MAX_BITMAPS 128

+#define SPLITPOINT_PHASES_PER_GRP 4
+#define SPLITPOINT_PHASE_MASK (SPLITPOINT_PHASES_PER_GRP - 1)
+#define SPLITPOINT_GROUPS_WITH_ONLY_ONE_PHASE 10
+#define HASH_MAX_SPLITPOINTS \
+ ((32 - SPLITPOINT_GROUPS_WITH_ONLY_ONE_PHASE) * \
+ SPLITPOINT_PHASES_PER_GRP) + \
+ SPLITPOINT_GROUPS_WITH_ONLY_ONE_PHASE

You have changed the value of HASH_MAX_SPLITPOINTS, but the comments
explaining that value are still unchanged.  Refer below text.
"The limitation on the size of spares[] comes from the fact that there's
 * no point in having more than 2^32 buckets with only uint32 hashcodes."



-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Logical decoding on standby

[Craig Ringer]

On 29 March 2017 at 16:44, Craig Ringer  wrote:

> * Split oldestCatalogXmin tracking into separate patch

Regarding this, Simon raised concerns about xlog volume here.

It's pretty negligible.

We only write a new record when a vacuum runs after catalog_xmin
advances on the slot with the currently-lowest catalog_xmin (or, if
vacuum doesn't run reasonably soon, when the bgworker next looks).

So at worst on a fairly slow moving system or one with a super high
vacuum rate we'll write one per commit. But in most cases we'll write
a lot fewer than that. When running t/006_logical_decoding.pl for
example:

$ ../../../src/bin/pg_waldump/pg_waldump
tmp_check/data_master_daPa/pgdata/pg_wal/00010001  |
grep CATALOG
rmgr: Transaction len (rec/tot):  4/30, tx:  0, lsn:
0/01648D50, prev 0/01648D18, desc: CATALOG_XMIN catalog_xmin 555
rmgr: Transaction len (rec/tot):  4/30, tx:  0, lsn:
0/0164C840, prev 0/0164C378, desc: CATALOG_XMIN catalog_xmin 0
pg_waldump: FATAL:  error in WAL record at 0/16BBF10: invalid record
length at 0/16BBF88: wanted 24, got 0


and of course, none at all unless you use logical decoding.



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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Allow interrupts on waiting standby

[Tsunakawa, Takayuki]

 (4) standby.c
> The latch is not reset when the wait timed out.  The next WaitLatch() would
> return immediately.

Sorry, let me withdraw this.  This is my misunderstanding.

OTOH, when is the latch reset before the wait?  Is there an assumption that 
MyLatch has been in reset state since it was initialized?
Is it safe to use MyLatch here, not the special latch like something used in 
xlog.c?


Regards
Takayuki Tsunakawa


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Logical decoding on standby

[Craig Ringer]

On 29 March 2017 at 08:11, Craig Ringer  wrote:
> On 29 March 2017 at 08:01, Craig Ringer  wrote:
>
>> I just notice that I failed to remove the docs changes regarding
>> dropping slots becoming db-specific, so I'll post a follow-up for that
>> in a sec.
>
> Attached.

... and here's the next in the patch series. Both this and the
immediately prior minor patch fix-drop-slot-docs.patch are pending
now.


Notable changes in this patch since review:

* Split oldestCatalogXmin tracking into separate patch

* Critically, fix use of procArray->replication_slot_catalog_xmin in
GetSnapshotData's setting of RecentGlobalXmin and RecentGlobalDataXmin
so it instead uses ShmemVariableCache->oldestCatalogXmin . This
could've led to tuples newer than oldestCatalogXmin being removed.

* Memory barrier in UpdateOldestCatalogXmin and SetOldestCatalogXmin.
It still does a pre-check before deciding if it needs to take
ProcArrayLock, recheck, and advance, since we don't want to
unnecessarily contest ProcArrayLock.

* Remove unnecessary volatile usage (retained in
UpdateOldestCatalogXmin due to barrier)

* Remove unnecessary test for XLogInsertAllowed() in XactLogCatalogXminUpdate

* EnsureActiveLogicalSlotValid(void)  - add (void)

* pgidented changes in this diff; have left unrelated changes alone




Re:

> what does
>
> +   TransactionId oldestCatalogXmin; /* oldest xid where complete catalog 
> state
> + * 
> is guaranteed to still exist */
>
> mean?  I complained about the overall justification in the commit
> already, but looking at this commit alone, the justification for this
> part of the change is quite hard to understand.

The patch now contains

TransactionId oldestCatalogXmin; /* oldest xid it is guaranteed to be safe
  * to create a historic snapshot for; see
  * also
  * procArray->replication_slot_catalog_xmin
  * */

which I think is an improvement.

I've also sought to explain the purpose of this change better with

/*
 * If necessary, copy the current catalog_xmin needed by replication slots to
 * the effective catalog_xmin used for dead tuple removal and write a WAL
 * record recording the change.
 *
 * This allows standbys to know the oldest xid for which it is safe to create
 * a historic snapshot for logical decoding. VACUUM or other cleanup may have
 * removed catalog tuple versions needed to correctly decode transactions older
 * than this threshold. Standbys can use this information to cancel conflicting
 * decoding sessions and invalidate slots that need discarded information.
 *
 * (We can't use the transaction IDs in WAL records emitted by VACUUM etc for
 * this, since they don't identify the relation as a catalog or not.  Nor can a
 * standby look up the relcache to get the Relation for the affected
 * relfilenode to check if it is a catalog. The standby would also have no way
 * to know the oldest safe position at startup if it wasn't in the control
 * file.)
 */
void
UpdateOldestCatalogXmin(void)
{
...

Does that help?




(Sidenote for later: ResolveRecoveryConflictWithLogicalDecoding will
need a read barrier too, when the next patch adds it.)

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
From 4b8e3aaa52539ef8cf3c79d1ed0319cc44800a32 Mon Sep 17 00:00:00 2001
From: Craig Ringer 
Date: Wed, 22 Mar 2017 13:36:49 +0800
Subject: [PATCH] Log catalog_xmin advances before removing catalog tuples

Write a WAL record before advancing the oldest catalog_xmin preserved by
VACUUM and other tuple removal.

Previously GetOldestXmin would use procArray->replication_slot_catalog_xmin as
the xid limit for vacuuming catalog tuples, so it was not possible for standbys
to determine whether all catalog tuples needed for a catalog snapshot for a
given xid would still exist.

Logging catalog_xmin advances allows standbys to determine if a logical slot on
the standby has become unsafe to use. It can then refuse to start logical
decoding on that slot or, if decoding is in progress, raise a conflict with
recovery.

Note that we only emit new WAL records if catalog_xmin changes, which happens
due to changes in slot state. So this won't generate WAL whenever oldestXmin
advances.
---
 src/backend/access/heap/rewriteheap.c   |   3 +-
 src/backend/access/rmgrdesc/xactdesc.c  |   9 +++
 src/backend/access/transam/varsup.c |  14 
 src/backend/access/transam/xact.c   |  35 
 src/backend/access/transam/xlog.c   |  12 ++-
 src/backend/commands/vacuum.c   |   9 +++
 src/backend/postmaster/bgwriter.c   |  10 +++
 src/backend/replication/logical/decode.c|  11 +++
 

Re: [HACKERS] Partitioned tables and relfilenode

[Kyotaro HORIGUCHI]

At Wed, 29 Mar 2017 17:21:26 +0900, Amit Langote 
 wrote in 

> > Thanks for taking a look.

This patch is small enough to look at in a short time:p

> > The following attracted my eyes.
> > 
> > +  if (def->defnamespace == NULL &&
> > +  pg_strcasecmp(def->defname, "oids") != 0)
> > +ereport(ERROR,
> > +  (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > +   errmsg("unrecognized parameter \"%s\" for a partitioned table",
> > 
> > This works since defnamespace is always NULL here, but if I
> > understand correctly what we should do here is "reject any option
> > other than "(default).OID"". So I think that the condition should
> > be like the following.
> 
> You're right.  The following *wrongly* succeeds:

Ouch! I briefly checked that by "hoge.oids" without confirming
around.

> create table p (a int) partition by list (a) with
> (toast.autovacuum_enabled = true);
> CREATE TABLE
> 
> > +  if (def->defnamespace != NULL ||
> > +  pg_strcasecmp(def->defname, "oids") != 0)
> 
> Looks correct, so incorporated in the attached updated patch.  Thanks.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] ANALYZE command progress checker

[vinayak]

On 2017/03/25 4:30, Robert Haas wrote:

On Fri, Mar 24, 2017 at 3:41 AM, vinayak
 wrote:

I have updated the patch.

You can't change the definition of AcquireSampleRowsFunc without
updating the documentation in fdwhandler.sgml, but I think I don't
immediately understand why that's a thing we want to do at all if
neither file_fdw nor postgres_fdw are going to use the additional
argument.  It seems to be entirely pointless because the new value
being passed down is always zero and even if the callee were to update
it, do_analyze_rel() would just ignore the change on return.  Am I
missing something here?  Also, the whitespace-only changes to fdwapi.h
related to AcquireSampleRowsFunc going to get undone by pgindent, so
even if there's some reason to change that you should leave the lines
that don't need changing untouched.
I Understood that we could not change the definition of 
AcquireSampleRowsFunc without updating the documentation in fdwhandler.sgml.
In the last patch I have modified the definition of 
AcquireSampleRowsFunc to handle the inheritance case.
If the table being analyzed has one or more children, ANALYZE will 
gather statistics twice:
once on the rows of parent table only and second on the rows of the 
parent table with all of its children.
So while reporting the progress the value of number of target sample 
rows and number of rows sampled is mismatched.

For single relation it works fine.

In the attached patch I have not change the definition of 
AcquireSampleRowsFunc.

I have updated inheritance case in the the documentation.



+/* Reset rows processed to zero for the next column */
+pgstat_progress_update_param(PROGRESS_ANALYZE_NUM_ROWS_PROCESSED,
0);

This seems like a bad design.  If you see a value other than zero in
this field, you still won't know how much progress analyze has made,
because you don't know which column you're processing.  I also feel
like you're not paying enough attention to a key point here that I
made in my last review, which is that you need to look at where
ANALYZE actually spends most of the time.  If the process of computing
the per-row statistics consumes significant CPU time, then maybe
there's some point in trying to instrument it, but does it?  Unless
you know the answer to that question, you don't know whether there's
even a point to trying to instrument this.

Understood. The computing statistics phase takes long time so I am 
looking at the code.


Regards,
Vinayak Pokale
NTT Open Source Software Center


pg_stat_progress_analyze_v7.patch
Description: binary/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

[Amit Langote]

On 2017/03/29 0:39, Robert Haas wrote:
> On Tue, Mar 28, 2017 at 6:50 AM, Amit Langote
>  wrote:
>>> Isn't it bogus that this is generating ALTER TABLE .. SET NOT NULL
>>> columns at all?  You didn't say anything like that when setting up the
>>> database, so why should it be there when dumping?
>>
>> So we should find a way for the NOT NULL constraints added for the range
>> partition key columns to not be emitted *separately*?  Like when a table
>> has primary key:
>>
>> --
>> -- Name: foo; Type: TABLE; Schema: public; Owner: amit
>> --
>>
>> CREATE TABLE foo (
>> a integer NOT NULL
>> );
>>
>>
>> ALTER TABLE foo OWNER TO amit;
>>
>> --
>> -- Name: foo foo_pkey; Type: CONSTRAINT; Schema: public; Owner: amit
>> --
>>
>> ALTER TABLE ONLY foo
>> ADD CONSTRAINT foo_pkey PRIMARY KEY (a);
>>
>> The NOT NULL constraint is emitted with CREATE TABLE, not separately.
> 
> Hmm, that's not exactly what I was thinking, but I think what I was
> thinking was wrong, so, yes, can we do what you said?

I thought about this for a while.  Although it seems we can do what I said
for (partitioned) tables themselves, it's not real clear to me how
straightforward it is to do for partitions (child tables). Inheritance or
localness of attributes/constraints including NOT NULL dictates whether an
attribute or a constraint is emitted separately.  I think that any
additional consideration will make the logic to *not* dump separately (or
perhaps to not emit at all) will become more involved.  For example, if a
NOT NULL constraint on a column has been inherited and originated in the
parent from the range partition key, then does it mean we should not emit
it or emit it but not separately?

Thanks,
Amit




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [patch] reorder tablespaces in basebackup tar stream for backup_label

[Kyotaro HORIGUCHI]

Hello,

At Wed, 29 Mar 2017 09:23:42 +0200, Michael Banck  
wrote in <149077.18436.14.ca...@credativ.de>
> Hi,
> 
> Am Mittwoch, den 29.03.2017, 15:22 +0900 schrieb Michael Paquier:
> > On Wed, Mar 29, 2017 at 3:56 AM, Fujii Masao  wrote:
> > > If your need other information except START WAL LOCATION at the beginning 
> > > of
> > > base backup and they are very useful for many third-party softwares,
> > > you can add them into that first result set. If you do this, you can
> > > retrieve them
> > > at the beginning even when WAL files are included in the backup.
> > 
> > You mean in the result tuple of pg_start_backup(), right? Why not.
> 
> The replication protocol chapter says: "When the backup is started, the
> server will first send two ordinary result sets, followed by one or more
> CopyResponse results. The first ordinary result set contains the
> starting position of the backup, in a single row with two columns."
> 
> However, I don't think it is very obvious to users (or at least it is
> not to me) how to get at this from psql, if you want to script it.  If I
> run something like 'psql "dbname=postgres replication=database" -c
> "BASE_BACKUP" "> foo', I seem to only get the tar file(s), unless I am
> missing something. 

Interesting. The protocol documentation seems fine to me but
maybe the example is perplexing. psql always prints only the last
result set for a query. So your query resembling the example in
the page gives only unrecognizable dump.

A little modification to psql prints the follwing but anyway
modifying psql to handle BASE_BACKUP like this doesn't seem
reasonable.

$ psql "dbname=postgres replication=database port=5433" -c "BASE_BACKUP"  | head
  recptr   | tli 
---+-
 0/C28 |   1
(1 row)

 spcoid | spclocation | size 
+-+--
| | 
(1 row)

backup_labelgres000E)
CHECKPOINT LOCATION: 0/E60
BACKUP METHOD: streamed
BACKUP FROM: master
START TIME: 2017-03-29 17:32:16 JST
LABEL: base backup


> The reason one might not want to run pg_basebackup directly is that this
> way one can pipe the output to an external program, e.g. to better
> compress it; this is not possible with pg_basebackup if you additional
> tablespaces.
> 
> So I think that has some merit on its own.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] On How To Shorten the Steep Learning Curve Towards PG Hacking...

[Kang Yuzhe]

Thanks Amit for further confirmation on the  Craig's intention.

I am looking forward to seeing your "PG internal machinery under
microscope" blog. May health, persistence and courage be with YOU.

Regards,
Zeray

On Wed, Mar 29, 2017 at 10:36 AM, Amit Langote <
langote_amit...@lab.ntt.co.jp> wrote:

> On 2017/03/29 12:36, Craig Ringer wrote:
> > On 29 March 2017 at 10:53, Amit Langote 
> wrote:
> >> Hi,
> >>
> >> On 2017/03/28 15:40, Kang Yuzhe wrote:
> >>> Thanks Tsunakawa for such an informative reply.
> >>>
> >>> Almost all of the docs related to the internals of PG are of
> introductory
> >>> concepts only.
> >>> There is even more useful PG internals site entitled "The Internals of
> >>> PostgreSQL" in http://www.interdb.jp/pg/ translation of the Japanese
> PG
> >>> Internals.
> >>>
> >>> The query processing framework that is described in the manual as you
> >>> mentioned is of informative and introductory nature.
> >>> In theory, the query processing framework described in the manual is
> >>> understandable.
> >>>
> >>> Unfortunate, it is another story to understand how query processing
> >>> framework in PG codebase really works.
> >>> It has become a difficult task for me to walk through the PG source
> code
> >>> for example how SELECT/INSERT/TRUNCATE in the the different modules
> under
> >>> "src/..". really works.
> >>>
> >>> I wish there were Hands-On with PostgreSQL Internals like
> >>> https://bkmjournal.wordpress.com/2017/01/22/hands-on-with-
> postgresql-internals/
> >>> for more complex PG features.
> >>>
> >>> For example, MERGE SQL standard is not supported yet by PG.  I wish
> there
> >>> were Hands-On with PostgreSQL Internals for MERGE/UPSERT. How it is
> >>> implemented in parser/executor/storage etc. modules with detailed
> >>> explanation for each code and debugging and other important concepts
> >>> related to system programming.
> >>
> >> I am not sure if I can show you that one place where you could learn all
> >> of that, but many people who started with PostgreSQL development at some
> >> point started by exploring the source code itself (either for learning
> or
> >> to write a feature patch), articles on PostgreSQL wiki, and many related
> >> presentations accessible using the Internet. I liked the following among
> >> many others:
> >
> > Personally I have to agree that the learning curve is very steep. Some
> > of the docs and presentations help, but there's a LOT to understand.
>
> I agree too. :)
>
> > When you're getting started you're lost in a world of language you
> > don't know, and trying to understand one piece often gets you lost in
> > other pieces. In no particular order:
> >
> > * Memory contexts and palloc
> > * Managing transactions and how that interacts with memory contexts
> > and the default memory context
> > * Snapshots, snapshot push/pop, etc
> > * LWLocks, memory barriers, spinlocks, latches
> > * Heavyweight locks (and the different APIs to them)
> > * GUCs, their scopes, the rules around their callbacks, etc
> > * dynahash
> > * catalogs and oids and access methods
> > * The heap AM like heap_open
> > * relcache, catcache, syscache
> > * genam and the systable_ calls and their limitations with indexes
> > * The SPI
> > * When to use each of the above 4!
> > * Heap tuples and minimal tuples
> > * VARLENA
> > * GETSTRUCT, when you can/can't use it, other attribute fetching methods
> > * TOAST and detoasting datums.
> > * forming and deforming tuples
> > * LSNs, WAL/xlog generation and redo. Timelines. (ARGH, timelines).
> > * cache invalidations, when they can happen, and how to do anything
> > safely around them.
> > * TIDs, cmin and cmax, xmin and xmax
> > * postmaster, vacuum, bgwriter, checkpointer, startup process,
> > walsender, walreceiver, all our auxillary procs and what they do
> > * relmapper, relfilenodes vs relation oids, filenode extents
> > * ondisk structure, page headers, pages
> > * shmem management, buffers and buffer pins
> > * bgworkers
> > * PG_TRY() and PG_CATCH() and their limitations
> > * elog and ereport and errcontexts, exception unwinding/longjmp and
> > how it interacts with memory contexts, lwlocks, etc
> > * The nest of macros around datum manipulation and functions, PL
> > handlers. How to find the macros for the data types you want to work
> > with.
> > * Everything to do with the C API for arrays (is horrible)
> > * The details of the parse/rewrite/plan phases with rewrite calling
> > back into parse, paths, the mess with inheritance_planner, reading and
> > understanding plantrees
> > * The permissions and grants model and how to interact with it
> > * PGPROC, PGXACT, other main shmem structures
> > * Resource owners (which I still don't fully "get")
> > * Checkpoints, pg_control and ShmemVariableCache, crash recovery
> > * How globals are used in Pg and how they interact with fork()ing from
> > postmaster
> > * SSI (haven't gone there yet myself)
> > * 
>
> That is indeed a 

Re: [HACKERS] Partitioned tables and relfilenode

[Amit Langote]

Horiguchi-san,

Thanks for taking a look.

On 2017/03/29 16:49, Kyotaro HORIGUCHI wrote:
> At Wed, 29 Mar 2017 15:40:20 +0900, Amit Langote wrote:
>> On 2017/03/27 23:27, Robert Haas wrote:
 And here is the updated patch.
>>>
>>> I think you should go back to the earlier strategy of disallowing heap
>>> reloptions to be set on the partitioned table.  The thing is, we don't
>>> really know which way we're going to want to go in the future.  Maybe
>>> we'll come up with a system where you can set options on the
>>> partitioned table, and those options will cascade to the children.  Or
>>> maybe we'll come up with a system where partitioned tables have a
>>> completely different set of options to control behaviors specific to
>>> partitioned tables.  If we do the latter, then we don't want to also
>>> have to support useless heap reloptions for forward compatibility, nor
>>> do we want to break backward-compatibility to remove support.  If we
>>> do the former, then it's better if we allow it in the same release
>>> where it starts working.
>>
>> You're right, modified the patch accordingly.
>>
>> By the way, the previous version of the patch didn't really "disallow"
>> specifying heap reloptions though.  What I'd imagine that should entail is
>> CREATE TABLE or ALTER TABLE raising error if one of those reloptions is
>> specified, which didn't really occur with the patch.  The options were
>> silently accepted and stored into pg_class, but their values were never
>> used.  I modified the patch so that an error occurs instead of silently
>> accepting the user input.
>>
>> create table p (a int) partition by list (a) with (fillfactor = 10);
>> ERROR:  unrecognized parameter "fillfactor" for a partitioned table
> 
> The following attracted my eyes.
> 
> +  if (def->defnamespace == NULL &&
> +  pg_strcasecmp(def->defname, "oids") != 0)
> +ereport(ERROR,
> +  (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> +   errmsg("unrecognized parameter \"%s\" for a partitioned table",
> 
> This works since defnamespace is always NULL here, but if I
> understand correctly what we should do here is "reject any option
> other than "(default).OID"". So I think that the condition should
> be like the following.

You're right.  The following *wrongly* succeeds:

create table p (a int) partition by list (a) with
(toast.autovacuum_enabled = true);
CREATE TABLE

> +  if (def->defnamespace != NULL ||
> +  pg_strcasecmp(def->defname, "oids") != 0)

Looks correct, so incorporated in the attached updated patch.  Thanks.

Regards,
Amit
>From 90a007dbf0eced97bb949da9a79df07b52486a60 Mon Sep 17 00:00:00 2001
From: amit 
Date: Tue, 24 Jan 2017 14:22:34 +0900
Subject: [PATCH] Do not allocate storage for partitioned tables.

Currently, it is not possible to insert any data into a partitioned
table.  So they're empty at all times, which means it is wasteful to
allocate relfilenode and related storage objects.

Also, because there is no storage, it doesn't make sense to allow
specifying storage parameters such as fillfactor, etc.

Patch by: Amit Langote & Maksim Milyutin
Reviwed by: Kyotaro Horiguchi
---
 doc/src/sgml/ref/create_table.sgml |  6 
 src/backend/access/common/reloptions.c |  3 +-
 src/backend/catalog/heap.c | 20 +++--
 src/backend/commands/tablecmds.c   | 45 ++
 src/test/regress/expected/alter_table.out  |  9 ++
 src/test/regress/expected/create_table.out |  5 
 src/test/regress/sql/alter_table.sql   |  7 +
 src/test/regress/sql/create_table.sql  |  4 +++
 8 files changed, 89 insertions(+), 10 deletions(-)

diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 283d53e203..5f13f240f5 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -1040,6 +1040,12 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI
 will use the table's parameter value.

 
+   
+Note that specifying the following parameters for partitioned tables is
+not supported.  You must specify them for individual leaf partitions if
+necessary.
+   
+

 

diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 72e12532ab..d9ea89db12 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -967,7 +967,6 @@ extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
 		case RELKIND_RELATION:
 		case RELKIND_TOASTVALUE:
 		case RELKIND_MATVIEW:
-		case RELKIND_PARTITIONED_TABLE:
 			options = heap_reloptions(classForm->relkind, datum, false);
 			break;
 		case RELKIND_VIEW:
@@ -976,6 +975,7 @@ extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
 		case RELKIND_INDEX:
 			options = index_reloptions(amoptions, datum, false);
 			break;
+		case RELKIND_PARTITIONED_TABLE:
 		case 

Re: [HACKERS] Logical replication existing data copy

[Erik Rijkers]

On 2017-03-09 11:06, Erik Rijkers wrote:


I use three different machines (2 desktop, 1 server) to test logical
replication, and all three have now at least once failed to correctly
synchronise a pgbench session (amidst many succesful runs, of course)





(At the moment using tese patches for tests:)


0001-Reserve-global-xmin-for-create-slot-snasphot-export.patch +
0002-Don-t-use-on-disk-snapshots-for-snapshot-export-in-l.patch+
0003-Prevent-snapshot-builder-xmin-from-going-backwards.patch  +
0004-Fix-xl_running_xacts-usage-in-snapshot-builder.patch  +
0005-Skip-unnecessary-snapshot-builds.patch+



The failed tests that I kept seeing (see the 
pgbench-over-logical-replication tests upthread) were never really 
'solved'.



But I have now finally figured out what caused these unexpected failed 
tests: it was  wal_sender_timeout  or rather, its default of 60 s.


This caused 'terminating walsender process due to replication timeout' 
on the primary (not strictly an error), and the concomittant ERROR on 
the replica: 'could not receive data from WAL stream: server closed the 
connection unexpectedly'.


here is a typical example (primary/replica logs time-intertwined, with 
'primary'):


[...]
2017-03-24 16:21:38.129 CET [15002]  primaryLOG:  using stale 
statistics instead of current ones because stats collector is not 
responding
2017-03-24 16:21:42.690 CET [27515]  primaryLOG:  using stale 
statistics instead of current ones because stats collector is not 
responding
2017-03-24 16:21:42.965 CET [14999]replica  LOG:  using stale 
statistics instead of current ones because stats collector is not 
responding
2017-03-24 16:21:49.816 CET [14930]  primaryLOG:  terminating 
walsender process due to
2017-03-24 16:21:49.817 CET [14926]replica  ERROR:  could not 
receive data from WAL stream: server closed the connection unexpectedly
2017-03-24 16:21:49.824 CET [27502]replica  LOG:  worker process: 
logical replication worker for subscription 24864 (PID 14926) exited 
with exit code 1
2017-03-24 16:21:49.824 CET [27521]replica  LOG:  starting logical 
replication worker for subscription "sub1"
2017-03-24 16:21:49.828 CET [15008]replica  LOG:  logical 
replication apply for subscription sub1 started
2017-03-24 16:21:49.832 CET [15009]  primaryLOG:  received 
replication command: IDENTIFY_SYSTEM
2017-03-24 16:21:49.832 CET [15009]  primaryLOG:  received 
replication command: START_REPLICATION SLOT "sub1" LOGICAL 3/FC976440 
(proto_version '1', publication_names '"pub1"')
2017-03-24 16:21:49.833 CET [15009]  primaryDETAIL:  streaming 
transactions committing after 3/FC889810, reading WAL from 3/FC820FC0
2017-03-24 16:21:49.833 CET [15009]  primaryLOG:  starting logical 
decoding for slot "sub1"
2017-03-24 16:21:50.471 CET [15009]  primaryDETAIL:  Logical 
decoding will begin using saved snapshot.
2017-03-24 16:21:50.471 CET [15009]  primaryLOG:  logical decoding 
found consistent point at 3/FC820FC0
2017-03-24 16:21:51.169 CET [15008]replica  DETAIL:  Key 
(hid)=(9014) already exists.
2017-03-24 16:21:51.169 CET [15008]replica  ERROR:  duplicate key 
value violates unique constraint "pgbench_history_pkey"
2017-03-24 16:21:51.170 CET [27502]replica  LOG:  worker process: 
logical replication worker for subscription 24864 (PID 15008) exited 
with exit code 1
2017-03-24 16:21:51.170 CET [27521]replica  LOG:  starting logical 
replication worker for subscription "sub1"

[...]

My primary and replica were always on a single machine (making it more 
likely that that timeout is reached?).  In my testing it seems that 
reaching the timeout on the primary (and 'closing the connection 
unexpectedly' on the replica) does not necessarily break the logical 
replication.  But almost all log-rep failures that I have seen were 
started by this sequence of events.


After setting  wal_sender_timeout  to 3 minutes there were no more 
failed tests.


Perhaps it warrants setting  wal_sender_timeout  a bit higher than the 
current default of 60 seconds?  After all I also saw the 'replication 
timeout' / 'closed the connection' couple rather often during 
not-failing tests.  (These also disappeared, almost completely,  with a 
higher  setting of wal_sender_timeout)


In any case it would be good to mention the setting (and its potentially 
deteriorating effect) somehere nearer the logical replication treatment.


( I read about wal_sender_timeout and keepalive ping, perhaps there's 
(still) something amiss there? Just a guess, I don't know )


As I said, I saw no more failures with the higher 3 minute setting, with 
one exception: the one test that straddled the DST change (saterday 24 
march 02:00 h).  I am happy to discount that one failure but strictly 
speaking I suppose it should be able to take DST into its stride.



Thanks,

Erik Rijkers











--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes 

[HACKERS] [PATCH] Remove trailing spaces

[Alexander Law]

Hello,

Please consider committing the attached patches to remove trailing 
spaces in strings in the source code.
One patch is for localizable messages, and the other is just for 
consistency (less important).


--
Alexander Lakhin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index bfe44b8..3010723 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -683,7 +683,7 @@ usage(void)
 	printf(_("%s decodes and displays PostgreSQL transaction logs for debugging.\n\n"),
 		   progname);
 	printf(_("Usage:\n"));
-	printf(_("  %s [OPTION]... [STARTSEG [ENDSEG]] \n"), progname);
+	printf(_("  %s [OPTION]... [STARTSEG [ENDSEG]]\n"), progname);
 	printf(_("\nOptions:\n"));
 	printf(_("  -b, --bkp-details  output detailed information about backup blocks\n"));
 	printf(_("  -e, --end=RECPTR   stop reading at log position RECPTR\n"));
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 27155f8..1e59fc8 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -3082,7 +3082,7 @@ keep_going:		/* We will come back to here until there is
 restoreErrorMessage(conn, );
 appendPQExpBuffer(>errorMessage,
   libpq_gettext("test \"show transaction_read_only\" failed "
-" on \"%s:%s\" \n"),
+" on \"%s:%s\"\n"),
   conn->connhost[conn->whichhost].host,
   conn->connhost[conn->whichhost].port);
 conn->status = CONNECTION_OK;
diff --git a/contrib/oid2name/oid2name.c b/contrib/oid2name/oid2name.c
index 778e8ba..ec93e4b 100644
--- a/contrib/oid2name/oid2name.c
+++ b/contrib/oid2name/oid2name.c
@@ -506,20 +506,20 @@ sql_exec_searchtables(PGconn *conn, struct options * opts)
 	/* now build the query */
 	todo = psprintf(
 	"SELECT pg_catalog.pg_relation_filenode(c.oid) as \"Filenode\", relname as \"Table Name\" %s\n"
-	"FROM pg_catalog.pg_class c \n"
-		"	LEFT JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace \n"
+	"FROM pg_catalog.pg_class c\n"
+		"	LEFT JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace\n"
 	"	LEFT JOIN pg_catalog.pg_database d ON d.datname = pg_catalog.current_database(),\n"
-	"	pg_catalog.pg_tablespace t \n"
+	"	pg_catalog.pg_tablespace t\n"
 	"WHERE relkind IN (" CppAsString2(RELKIND_RELATION) ","
 	CppAsString2(RELKIND_MATVIEW) ","
 	CppAsString2(RELKIND_INDEX) ","
 	CppAsString2(RELKIND_SEQUENCE) ","
-	CppAsString2(RELKIND_TOASTVALUE) ") AND \n"
+	CppAsString2(RELKIND_TOASTVALUE) ") AND\n"
 	"		t.oid = CASE\n"
 			"			WHEN reltablespace <> 0 THEN reltablespace\n"
 	"			ELSE dattablespace\n"
-	"		END AND \n"
-	"  (%s) \n"
+	"		END AND\n"
+	"  (%s)\n"
 	"ORDER BY relname\n",
 	opts->extended ? addfields : "",
 	qualifiers);
diff --git a/src/backend/executor/nodeMergejoin.c b/src/backend/executor/nodeMergejoin.c
index 62784af..7c0725c 100644
--- a/src/backend/executor/nodeMergejoin.c
+++ b/src/backend/executor/nodeMergejoin.c
@@ -587,7 +587,7 @@ ExecMergeTupleDump(MergeJoinState *mergestate)
 	ExecMergeTupleDumpInner(mergestate);
 	ExecMergeTupleDumpMarked(mergestate);
 
-	printf(" \n");
+	printf("\n");
 }
 #endif
 
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 262f553..91319a8 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -13900,12 +13900,12 @@ dumpTSConfig(Archive *fout, TSConfigInfo *cfginfo)
 
 	resetPQExpBuffer(query);
 	appendPQExpBuffer(query,
-	  "SELECT \n"
-	  "  ( SELECT alias FROM pg_catalog.ts_token_type('%u'::pg_catalog.oid) AS t \n"
-	  "WHERE t.tokid = m.maptokentype ) AS tokenname, \n"
-	  "  m.mapdict::pg_catalog.regdictionary AS dictname \n"
-	  "FROM pg_catalog.pg_ts_config_map AS m \n"
-	  "WHERE m.mapcfg = '%u' \n"
+	  "SELECT\n"
+	  "  ( SELECT alias FROM pg_catalog.ts_token_type('%u'::pg_catalog.oid) AS t\n"
+	  "WHERE t.tokid = m.maptokentype ) AS tokenname,\n"
+	  "  m.mapdict::pg_catalog.regdictionary AS dictname\n"
+	  "FROM pg_catalog.pg_ts_config_map AS m\n"
+	  "WHERE m.mapcfg = '%u'\n"
 	  "ORDER BY m.mapcfg, m.maptokentype, m.mapseqno",
 	  cfginfo->cfgparser, cfginfo->dobj.catId.oid);
 
diff --git a/src/bin/pg_rewind/libpq_fetch.c b/src/bin/pg_rewind/libpq_fetch.c
index d86ecd3..eb74d2f 100644
--- a/src/bin/pg_rewind/libpq_fetch.c
+++ b/src/bin/pg_rewind/libpq_fetch.c
@@ -489,7 +489,7 @@ libpq_executeFileMap(filemap_t *map)
 	 * temporary table. Now, actually fetch all of those ranges.
 	 */
 	sql =
-		"SELECT path, begin, \n"
+		"SELECT path, begin,\n"
 		"  pg_read_binary_file(path, begin, len, true) AS chunk\n"
 		"FROM fetchchunks\n";
 
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index b0f3e5e..a623630 100644
--- a/src/bin/psql/describe.c
+++