Re: Small run-time pruning doc fix

[David Rowley]

Thanks for looking at this.

On 2 November 2018 at 20:30, Peter Eisentraut
 wrote:
>  
> - Execution-time partition pruning currently occurs for the
> + Execution-time partition pruning currently only occurs for the
>   Append and MergeAppend node
> types.
> + It is not yet implemented for the ModifyTable node
> + type.
>  
>
> Isn't this implied by the preceding paragraph
>
> Currently, pruning of partitions during the planning of an UPDATE or
> DELETE command is implemented using the constraint exclusion method

That paragraph is talking about plan-time partition pruning. The
paragraph the patch changes is mentioning execution-time partition
pruning.  In PG11 we have plan-time partition pruning for Append and
MergeAppend. UPDATE/DELETE does pruning, but it uses the constraint
exclusion code which does not work for HASH partitioned tables.  In
PG11 we only have run-time partition pruning for Append.   MergeAppend
support was only added in 5220bb7533 (PG12).

> Also, could there be other node types that could benefit from partition
> pruning that are not implemented yet?  Not sure we want to mention all
> of them.

I'm unsure what might exist in the future, but there are currently
only 3 node types that take a list of subpaths. I think if we get the
support for ModifyTable later, then likely the paragraph edited in
this patch would disappear. In fact, the entire note would disappear
as adding run-time pruning for ModifyTable will require the planner to
use the new partition pruning code.  There's a patch for that in [1],
so maybe it'll all be a little less confusing in PG12.

[1] https://commitfest.postgresql.org/20/1778/

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

Re: Making all nbtree entries unique by having heap TIDs participate in comparisons

[Peter Geoghegan]

On Sat, Nov 3, 2018 at 8:52 PM Andrey Lepikhov
 wrote:
> I applied your patches at top of master. After tests corrections
> (related to TID ordering in index relations DROP...CASCADE operation)
> 'make check-world' passed successfully many times.
> In the case of 'create view' regression test - 'drop cascades to 62
> other objects' problem - I verify an Álvaro Herrera hypothesis [1] and
> it is true. You can verify it by tracking the
> object_address_present_add_flags() routine return value.

I'll have to go and fix the problem directly, so that ASC sort order
can be used.

> Some doubts, however, may be regarding the 'triggers' test.
> May you specify the test failures do you mean?

Not sure what you mean. The order of items that are listed in the
DETAIL for a cascading DROP can have an "implementation defined"
order. I think that this is an example of the more general problem --
what you call the 'drop cascades to 62 other objects' problem is a
more specific subproblem, or, if you prefer, a more specific symptom
of the same problem.

Since I'm going to have to fix the problem head-on, I'll have to study
it in detail anyway.

-- 
Peter Geoghegan

Re: Making all nbtree entries unique by having heap TIDs participate in comparisons

[Peter Geoghegan]

On Fri, Nov 2, 2018 at 5:00 PM Peter Geoghegan  wrote:
> I had the opportunity to discuss this patch at length with Heikki
> during pgConf.EU.

> The DESC heap TID sort order thing probably needs to go. I'll probably
> have to go fix the regression test failures that occur when ASC heap
> TID order is used.

I've found that TPC-C testing with ASC heap TID order fixes the
regression that I've been concerned about these past few weeks. Making
this change leaves the patch a little bit faster than the master
branch for TPC-C, while still leaving TPC-C indexes about as small as
they were with v6 of the patch (i.e. much smaller). I now get about a
1% improvement in transaction throughput, an improvement that seems
fairly consistent. It seems likely that the next revision of the patch
series will be an unambiguous across the board win for performance. I
think that I come out ahead with ASC heap TID order because that has
the effect of reducing the volume of WAL generated by page splits.
Page splits are already optimized for splitting right, not left.

I should thank Heikki for pointing me in the right direction here.

--
Peter Geoghegan

Re: Tid scan improvements

[Edmund Horner]

Hi all,

I have managed to split my changes into 4 patches:

v3-0001-Add-selectivity-and-nullness-estimates-for-the-ItemP.patch
v3-0002-Support-range-quals-in-Tid-Scan.patch
v3-0003-Support-backward-scans-over-restricted-ranges-in-hea.patch
v3-0004-Tid-Scan-results-are-ordered.patch

(1) is basically independent, and usefully improves estimates for ctid quals.
(2) is the main patch, adding basic range scan support to TidPath and TidScan.
(3) is a small change to properly support backward scans over a
restricted range in heapam.c, and is needed for (4).
(4) adds Backward Tid Scans, and adds path keys to Tid Paths so that
the planner doesn't have to add a sort for certain queries.

I have tried to apply David's suggestions.

In (1), I've included the offset part of a CTID constant in the
selectivity calculation.  I've not included "allvisfrac" in the
calculation; I'm not sure it's worth it as it would only affect the
offset part.
I have tried to use iseq to differentiate between <=,>= versus <,>,
but I'm not sure I've got this right.  I am also not entirely sure
it's worth it; the changes are already an improvement over the current
behaviour of using hardcoded selectivity constants.

In (2), the planner now picks up a greater variety of TID quals,
including AND-clauses with arbitrary children instead of the original
lower bound/upper bound pair.  These are resolved in the executor into
a list of ranges to scan.

(3) is the same code, but I've added a couple of comments to explain the change.

(4) is basically the same pathkey/direction code as before (but as a
separate patch).

I hope the separation will make it easier to review.  Item (2) is
still quite big, but a third of it is tests.

Cheers.
Edmund


v3-0002-Support-range-quals-in-Tid-Scan.patch
Description: Binary data


v3-0001-Add-selectivity-and-nullness-estimates-for-the-ItemP.patch
Description: Binary data


v3-0003-Support-backward-scans-over-restricted-ranges-in-hea.patch
Description: Binary data


v3-0004-Tid-Scan-results-are-ordered.patch
Description: Binary data

Re: WIP: Avoid creation of the free space map for small tables

[Amit Kapila]

On Sun, Nov 4, 2018 at 5:56 AM Michael Paquier  wrote:
>
> On Fri, Nov 02, 2018 at 10:38:45AM -0400, Robert Haas wrote:
> > I think it's in evidence, in the form of several messages mentioning a
> > flag called try_every_block.
> >
> > Just checking the last page of the table doesn't sound like a good
> > idea to me.  I think that will just lead to a lot of stupid bloat.  It
> > seems likely that checking every page of the table is fine for npages
> > <= 3, and that would still be win in a very significant number of
> > cases, since lots of instances have many empty or tiny tables.  I was
> > merely reacting to the suggestion that the approach should be used for
> > npages <= 32; that threshold sounds way too high.
>
> It seems to me that it would be costly for schemas which have one core
> table with a couple of records used in many joins with other queries.
> Imagine for example a core table like that:
> CREATE TABLE us_states (id serial, initials varchar(2));
> INSERT INTO us_states VALUES (DEFAULT, 'CA');
>
> If there is a workload where those initials need to be fetched a lot,
> this patch could cause a loss.
>

How alone fetching would cause any loss? If it gets updated, then
there is a chance that we might have some performance impact.

>  It looks hard to me to put a straight
> number on when not having the FSM is better than having it because that
> could be environment-dependent, so there is an argument for making the
> default very low, still configurable?
>

I think 3 or 4 as threshold should work fine (though we need to
thoroughly test that) as we will anyway avoid having three additional
pages of FSM for such tables.  I am not sure how easy it would be for
users to set this value if we make it configurable or on what basis
can they configure?

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

Re: Making all nbtree entries unique by having heap TIDs participate in comparisons

[Andrey Lepikhov]

On 03.11.2018 5:00, Peter Geoghegan wrote:

The DESC heap TID sort order thing probably needs to go. I'll probably
have to go fix the regression test failures that occur when ASC heap
TID order is used. (Technically those failures are a pre-existing
problem, a problem that I mask by using DESC order...which is weird.
The problem is masked in the master branch by accidental behaviors
around nbtree duplicates, which is something that deserves to die.
DESC order is closer to the accidental current behavior.)


I applied your patches at top of master. After tests corrections 
(related to TID ordering in index relations DROP...CASCADE operation) 
'make check-world' passed successfully many times.
In the case of 'create view' regression test - 'drop cascades to 62 
other objects' problem - I verify an Álvaro Herrera hypothesis [1] and 
it is true. You can verify it by tracking the 
object_address_present_add_flags() routine return value.

Some doubts, however, may be regarding the 'triggers' test.
May you specify the test failures do you mean?

[1] 
https://www.postgresql.org/message-id/20180504022601.fflymidf7eoencb2%40alvherre.pgsql


--
Andrey Lepikhov
Postgres Professional
https://postgrespro.com
The Russian Postgres Company

Re: bugfix: BUG #15477: Procedure call with named inout refcursor parameter - "invalid input syntax for type boolean"

[Pavel Stehule]

Hi

so 3. 11. 2018 v 22:47 odesílatel Tom Lane  napsal:

> I wrote:
> > I'm going to go see about converting this to just call
> > expand_function_arguments and then drop all the special-case code.
>
> So while looking at that ... isn't the behavior for non-writable output
> parameters basically insane?  It certainly fails to accord with the
> plpgsql documentation, which shows an example that would throw an error:
>
> CREATE PROCEDURE triple(INOUT x int)
> ...
> CALL triple(5);
>
> It's even weirder that you can get away with not supplying a writable
> target value for an output argument so long as it has a default.
>
> I think the behavior here ought to be "if the actual argument is a plpgsql
> variable, assign the output back to it, otherwise do nothing".  That's
> much closer to the behavior of OUT arguments in other old-school
> programming languages.
>

I don't agree. The constant can be used only for IN parameter. Safe
languages like Ada does copy result to variable used as INOUT parameter.
PL/SQL doesn't allow it too.

https://stackoverflow.com/questions/9977229/oracle-pls-00363-expression-cannot-be-used-as-an-assignment-target

The implemented limit can be good detection of passing parameters in bad
order.

Regards

Pavel


>
> regards, tom lane
>

Re: settings to control SSL/TLS protocol version

[Steve Singer]

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

I've reviewed the patch and here are my comments.

The feature seems useful a lot of application servers are implementing minimal 
TLS protocol versions.
I don't see a way to restrict libpq to only connect with certain protocol 
versions.  Maybe that is a separate patch but it would make this feature harder 
to test in the future.

I tested with a server configured to via the options to only TLS1.3 and clients 
without TLSv1.3 support and confirmed that I couldn't connect with SSL. This is 
fine
I tested with options to restrict the max version to TLSv1 and verified that 
the clients connected with TLSv1. This is fine
I tested with a min protocol version greater than the max.  The server started 
up (Do we want this to be an warning on startup?) but I wasn't able to connect 
with SSL. The following was in the server log

could not accept SSL connection: unknown protocol

I tested with a max protocol version set to any. This is fine.
I tested putting TLSv1.3 in the config file when my openssl library did not 
support 1.3. This is fine.


I am updating the patch status to ready for committer.

The new status of this patch is: Ready for Committer

RE: [PATCH] Improvements to "Getting started" tutorial for Google Code-in task

[LAM JUN RONG]

Hi,

I have made some changes based on Andreas’ suggestions.

> +then one or more of the packages PostgreSQL 
> requires is not installed.
> +See  for the required packages.
>
> How about:
> then packages which are required to build
> PostgreSQL are missing.
> See  for a list of requirements.

This seems better than mine, I have included it.

>  If you are not sure whether PostgreSQL
> -is already available or whether you can use it for your
> -experimentation then you can install it yourself.  Doing so is not
> +is already available for your experimentation,
> +you can install it yourself.  Doing so is not
>  hard and it can be a good exercise.
>
> This change does not make much sense, given that you leave the
> second part of the sentence.

How’s this:
If you are not sure whether PostgreSQL
is already available for your experimentation,
you can install it yourself, which is not hard.

>  As is typical of client/server applications, the client and the
> -server can be on different hosts.  In that case they communicate
> +server can be on different machines or networks.  In that case they 
> communicate
>  over a TCP/IP network connection.  You should keep this in mind,
> I think "hosts" is preferred here. The differentiation between machines
> and networks is not necessary.
>
>
>
> -The path at your site might be different.  Contact your site
> +The path at your site's server might be different.  Contact your site
>  administrator or check the installation instructions to
> Dunno if this change improves the wording, or the understanding.
> How about replacing "site" with "installation"?

For the 2 points above, I decided that the original documentation seems fine.

>  PostgreSQL allows you to create any
> -number of databases at a given site.  Database names must have an
> -alphabetic first character and are limited to 63 bytes in
> -length.  A convenient choice is to create a database with the same
> -name as your current user name.  Many tools assume that database
> -name as the default, so it can save you some typing.  To create
> -that database, simply type:
> +number of databases at a given site.  Database names are limited to 63 
> bytes in
> +length. Database names longer than 63 bytes will be truncated. A 
> convenient
> +choice is to create a database with the same name as your current user 
> name.
> +Many tools assume that database name as the default, so it
> +can save you some typing. To create that database, simply type:
> The part about "truncate" is correct, maybe you can include NAMEDATALEN here 
> as well.
> The part about the first character is correct - except you quote the name.
> Then any name is allowed. If you rewrite this part, maybe include this as 
> well.

I’ve made some changes to include the part about NAMEDATALEN and quoting:
However, if you would like to create databases with
names that do not start with an alphabetic character, you will need to quote it 
like so: "1234".
Database names are limited to 63 bytes in length. Database names longer than 63 
bytes will be
truncated. You can change this limit by modifying the NAMEDATALEN variable,
but that would require recompiling PostgreSQL.


A full diff is attached.

Thank you,
Jun Rong
From: LAM JUN RONG
Sent: Sunday, November 4, 2018 6:17 AM
Subject: Re: [PATCH] Improvements to "Getting started" tutorial for Google 
Code-in task

Thanks for your feedback.
I'll make some changes based on your suggestions and send a new diff.
Thanks!
From: Andreas 'ads' Scherbaum 
Sent: Saturday, November 3, 2018 9:17:54 PM
To: LAM JUN RONG; pgsql-hack...@postgresql.org
Subject: Re: [PATCH] Improvements to "Getting started" tutorial for Google 
Code-in task

Hello,
On 02.11.18 14:07, LAM JUN RONG wrote:

I’m a student taking part in Google Code-in 2018. The 
task I am currently working on, 
https://codein.withgoogle.com/dashboard/task-instances/6406170207059968/, 
requires that I review and improve the “Getting Started” tutorial in the 
PostgreSQL docs, and submit a patch to this mailing list.

Thank you for picking up this task.


After reviewing the documentation, I found some parts to be slightly unclear. 
For example, in section 1.3 on creating databases, I found “no response” a bit 
unclear or ambiguous, so I replaced it with “exit without any error messages”.

After some experimentation, I found that a part of the documentation was 
incorrect. In Section 1.3, it was stated that “Database names must have an 
alphabetic first character”. However, I was able to create databases with names 
like “1234”, “$” or even “”. Hence, I decided to remove that sentence.

A diff of my changes is attached.

+then one or more of the packages PostgreSQL 
requires is not installed.
+See  for the required packages.

How about:
then packages which are required to build
PostgreSQL are 

Re: heap_sync seems rather oblivious to partitioned tables (wal_level=minimal)

[David Rowley]

On 2 November 2018 at 15:43, Steve Singer  wrote:
> I am happy with the changes.
> I think the patch is ready for a committer.

Many thanks for your review.


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

Re: New function pg_stat_statements_reset_query() to reset statistics of a specific query

[Haribabu Kommi]

On Sun, Nov 4, 2018 at 11:17 AM Michael Paquier  wrote:

> On Sat, Nov 03, 2018 at 03:56:14PM +0530, Amit Kapila wrote:
> > Before trying out any solution or deciding which is better, I think we
> > want to understand why the variability in results occurred only after
> > your patch?  Without the patch, it works just fine.
>
> Good point.  We surely want to have a stable feature, which gets tested
> without triggering random failures in the builfarm.
>

Thanks for the review.

This patch has changed the pg_stat_statements_reset() function from
returning void
to number statements that it reset. The regression test contains
pg_stat_statements_reset()
as first query to reset any of the query stats that are already tracked to
let the test to
provide the proper results. But with this feature, if we test this
regression test on an
already running server, the first query result is varying and it leads to
test failure.

So to fix this problem, I added a wrapper function that masks the result of
the
pg_stat_statements_reset() function and just return as void, with this
wrapper function
used a first statement, the test is stable, as this function takes care of
resetting already
existing statements from the already running server.

With the above change, the regression test is stable. Comments?

Regards,
Haribabu Kommi
Fujitsu Australia

Re: Resetting PGPROC atomics in ProcessInit()

[Robert Haas]

On Sat, Oct 27, 2018 at 6:41 AM Andres Freund  wrote:
> I just noticed, while working on a patch adding things to PGPROC, that
> the group clearning patches for the proc array and clog reset atomics in
> InitProcess().
>
> I'm not a big fan of that, because it means that it's not safe to look
> at the atomics of backends that aren't currently in use.  Is there any
> reason to not instead initialize them in InitProcGlobal() and just
> assert in InitProcess() that they're 0?  If they're not, we'd be in deep
> trouble anyway, no?

I think you are correct.  I think it would be better in general for
InitProcess() to Assert() rather than reinitializing.  Apart from this
issue, it's not free.

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

Re: CF app feature request

[Michael Paquier]

On Fri, Nov 02, 2018 at 09:15:36PM +0100, Dmitry Dolgov wrote:
> Just to make sure, if a duplicated entry will be removed, the patch itself
> will stay or not? I'm asking, because both entries have the same patch
> referenced, and the admin form says that one of the related items, that
> would be removed is the patch item.

If you remove only one entry, its references will be removed but the
second one will remain.  If you want me to proceed, I can do so.  I have
done that in the past, and it is not the first time someone registers a
duplicated entry in the CF app.
--
Michael


signature.asc
Description: PGP signature

Re: WIP: Avoid creation of the free space map for small tables

[Michael Paquier]

On Fri, Nov 02, 2018 at 10:38:45AM -0400, Robert Haas wrote:
> I think it's in evidence, in the form of several messages mentioning a
> flag called try_every_block.
> 
> Just checking the last page of the table doesn't sound like a good
> idea to me.  I think that will just lead to a lot of stupid bloat.  It
> seems likely that checking every page of the table is fine for npages
> <= 3, and that would still be win in a very significant number of
> cases, since lots of instances have many empty or tiny tables.  I was
> merely reacting to the suggestion that the approach should be used for
> npages <= 32; that threshold sounds way too high.

It seems to me that it would be costly for schemas which have one core
table with a couple of records used in many joins with other queries.
Imagine for example a core table like that:
CREATE TABLE us_states (id serial, initials varchar(2));
INSERT INTO us_states VALUES (DEFAULT, 'CA');

If there is a workload where those initials need to be fetched a lot,
this patch could cause a loss.  It looks hard to me to put a straight
number on when not having the FSM is better than having it because that
could be environment-dependent, so there is an argument for making the
default very low, still configurable?
--
Michael


signature.asc
Description: PGP signature

Re: New function pg_stat_statements_reset_query() to reset statistics of a specific query

[Michael Paquier]

On Sat, Nov 03, 2018 at 03:56:14PM +0530, Amit Kapila wrote:
> Before trying out any solution or deciding which is better, I think we
> want to understand why the variability in results occurred only after
> your patch?  Without the patch, it works just fine.

Good point.  We surely want to have a stable feature, which gets tested
without triggering random failures in the builfarm.
--
Michael


signature.asc
Description: PGP signature

Re: partitioned indexes and tablespaces

[Michael Paquier]

On Sat, Nov 03, 2018 at 12:30:15PM -0700, Andres Freund wrote:
> On 2018-11-03 16:24:28 -0300, Alvaro Herrera wrote:
>> On 2018-Nov-03, Robert Haas wrote:
>>> Well, you've guaranteed that already.  Now 11 will be different from
>>> 11.1, and tables will be different from indexes until somebody goes
>>> and makes that consistent again.
>> 
>> Nobody is running 11.0 in production.
> 
> Uh, I know of people doing so.

My understanding of the term GA is that anything marked as GA is
considered enough stable for production use, and that beta releases are
here for testing.  So I don't get the argument.  And you have made
partitioned indexes now inconsistent with partitioned tables.

Seeing this stuff discussed and committed in haste gives me the sad
face.

:(
--
Michael


signature.asc
Description: PGP signature

Re: [PATCH] Improvements to "Getting started" tutorial for Google Code-in task

[LAM JUN RONG]

Thanks for your feedback.

I'll make some changes based on your suggestions and send a new diff.

Thanks!
From: Andreas 'ads' Scherbaum 
Sent: Saturday, November 3, 2018 9:17:54 PM
To: LAM JUN RONG; pgsql-hack...@postgresql.org
Subject: Re: [PATCH] Improvements to "Getting started" tutorial for Google 
Code-in task


Hello,

On 02.11.18 14:07, LAM JUN RONG wrote:


I’m a student taking part in Google Code-in 2018. The 
task I am currently working on, 
https://codein.withgoogle.com/dashboard/task-instances/6406170207059968/, 
requires that I review and improve the “Getting Started” tutorial in the 
PostgreSQL docs, and submit a patch to this mailing list.


Thank you for picking up this task.



After reviewing the documentation, I found some parts to be slightly unclear. 
For example, in section 1.3 on creating databases, I found “no response” a bit 
unclear or ambiguous, so I replaced it with “exit without any error messages”.



After some experimentation, I found that a part of the documentation was 
incorrect. In Section 1.3, it was stated that “Database names must have an 
alphabetic first character”. However, I was able to create databases with names 
like “1234”, “$” or even “”. Hence, I decided to remove that sentence.



A diff of my changes is attached.


+then one or more of the packages PostgreSQL 
requires is not installed.
+See  for the required packages.


How about:

then packages which are required to build
PostgreSQL are missing.
See  for a list of requirements.




 If you are not sure whether PostgreSQL
-is already available or whether you can use it for your
-experimentation then you can install it yourself.  Doing so is not
+is already available for your experimentation,
+you can install it yourself.  Doing so is not
 hard and it can be a good exercise.


This change does not make much sense, given that you leave the
second part of the sentence.



 As is typical of client/server applications, the client and the
-server can be on different hosts.  In that case they communicate
+server can be on different machines or networks.  In that case they 
communicate
 over a TCP/IP network connection.  You should keep this in mind,

I think "hosts" is preferred here. The differentiation between machines
and networks is not necessary.




-The path at your site might be different.  Contact your site
+The path at your site's server might be different.  Contact your site
 administrator or check the installation instructions to

Dunno if this change improves the wording, or the understanding.
How about replacing "site" with "installation"?



 PostgreSQL allows you to create any
-number of databases at a given site.  Database names must have an
-alphabetic first character and are limited to 63 bytes in
-length.  A convenient choice is to create a database with the same
-name as your current user name.  Many tools assume that database
-name as the default, so it can save you some typing.  To create
-that database, simply type:
+number of databases at a given site.  Database names are limited to 63 
bytes in
+length. Database names longer than 63 bytes will be truncated. A convenient
+choice is to create a database with the same name as your current user 
name.
+Many tools assume that database name as the default, so it
+can save you some typing. To create that database, simply type:

The part about "truncate" is correct, maybe you can include NAMEDATALEN here as 
well.

The part about the first character is correct - except you quote the name.
Then any name is allowed. If you rewrite this part, maybe include this as well.


The other changes look good.

-- Andreas 'ads' Scherbaum German PostgreSQL User Group European PostgreSQL 
User Group - Board of Directors Volunteer Regional Contact, Germany - 
PostgreSQL Project

Reduce maintenance burden of alternative output files with \if \quit

[Andres Freund]

Hi,

We have a few alterntive expected output files that are essentially full
of errors, because a certain feature isn't supported. Those are somewhat
painful to maintain.  I wonder if it'd be a good idea to reduce the
maintenance overhead for some of them by putting something like

SELECT NOT feature_check_expr() AS dont_have_feature
\gset
\if :dont_have_feature
\quit
\endif

at the start of such regression tests.  Then the alternative
'dont-have-the-feature' output file will stay the same when adding new
tests.

It's a bit annoying that when running such a sequence interactively one
can hit
if (pset.cur_cmd_interactive && !active_branch &&
!is_branching_command(cmd))
{
psql_error("\\%s command ignored; use \\endif or Ctrl-C to exit 
current \\if block\n",
   cmd);
}
but given that's just when running interactively, it shouldn't be too
bad.

Greetings,

Andres Freund

Re: bugfix: BUG #15477: Procedure call with named inout refcursor parameter - "invalid input syntax for type boolean"

[Tom Lane]

I wrote:
> I'm going to go see about converting this to just call
> expand_function_arguments and then drop all the special-case code.

So while looking at that ... isn't the behavior for non-writable output
parameters basically insane?  It certainly fails to accord with the
plpgsql documentation, which shows an example that would throw an error:

CREATE PROCEDURE triple(INOUT x int)
...
CALL triple(5);

It's even weirder that you can get away with not supplying a writable
target value for an output argument so long as it has a default.

I think the behavior here ought to be "if the actual argument is a plpgsql
variable, assign the output back to it, otherwise do nothing".  That's
much closer to the behavior of OUT arguments in other old-school
programming languages.

regards, tom lane

Re: [PATCH] Improvements to "Getting started" tutorial for Google Code-in task

[Andreas 'ads' Scherbaum]

Hello,

On 02.11.18 14:07, LAM JUN RONG wrote:



I’m a student taking part in Google Code-in 2018. The 
 task I am currently working on, 
https://codein.withgoogle.com/dashboard/task-instances/6406170207059968/, 
requires that I review and improve the “Getting Started” tutorial in 
the PostgreSQL docs, and submit a patch to this mailing list.




Thank you for picking up this task.




After reviewing the documentation, I found some parts to be slightly 
unclear. For example, in section 1.3 on creating databases, I found 
“no response” a bit unclear or ambiguous, so I replaced it with “exit 
without any error messages”.




After some experimentation, I found that a part of the documentation 
was incorrect. In Section 1.3, it was stated that “Database names must 
have an alphabetic first character”. However, I was able to create 
databases with names like “1234”, “$” or even “”. Hence, I decided 
to remove that sentence.




A diff of my changes is attached.



+    then one or more of the packages 
PostgreSQL requires is not installed.

+    See  for the required packages.


How about:

then packages which are required to build
PostgreSQL are missing.
See  for a list of requirements.




 If you are not sure whether PostgreSQL
-    is already available or whether you can use it for your
-    experimentation then you can install it yourself.  Doing so is not
+    is already available for your experimentation,
+    you can install it yourself.  Doing so is not
 hard and it can be a good exercise.


This change does not make much sense, given that you leave the
second part of the sentence.



 As is typical of client/server applications, the client and the
-    server can be on different hosts.  In that case they communicate
+    server can be on different machines or networks.  In that case they 
communicate

 over a TCP/IP network connection.  You should keep this in mind,

I think "hosts" is preferred here. The differentiation between machines
and networks is not necessary.




-    The path at your site might be different.  Contact your site
+    The path at your site's server might be different.  Contact your site
 administrator or check the installation instructions to

Dunno if this change improves the wording, or the understanding.
How about replacing "site" with "installation"?



 PostgreSQL allows you to create any
-    number of databases at a given site.  Database names must have an
-    alphabetic first character and are limited to 63 bytes in
-    length.  A convenient choice is to create a database with the same
-    name as your current user name.  Many tools assume that database
-    name as the default, so it can save you some typing.  To create
-    that database, simply type:
+    number of databases at a given site.  Database names are limited to 
63 bytes in
+    length. Database names longer than 63 bytes will be truncated. A 
convenient
+    choice is to create a database with the same name as your current 
user name.

+    Many tools assume that database name as the default, so it
+    can save you some typing. To create that database, simply type:

The part about "truncate" is correct, maybe you can include NAMEDATALEN 
here as well.


The part about the first character is correct - except you quote the name.
Then any name is allowed. If you rewrite this part, maybe include this 
as well.



The other changes look good.

--
Andreas 'ads' Scherbaum
German PostgreSQL User Group
European PostgreSQL User Group - Board of Directors
Volunteer Regional Contact, Germany - PostgreSQL Project

Re: Installation instructions update (pg_ctl)

[Andreas 'ads' Scherbaum]

On 30.10.18 14:52, Andreas 'ads' Scherbaum wrote:

On 30.10.18 09:04, Michael Banck wrote:

Hi,

On Tue, Oct 30, 2018 at 12:08:49AM +0100, Andreas 'ads' Scherbaum wrote:
The installation instructions (short version) are not consistent 
with the

"initdb" output. The first one still uses "postgres -D", even mentions
"check initdb output", but "initdb" emits "pg_ctl" commands.

The attached patch updates the short install instructions and replaces
"postgres -D" with "pg_ctl" calls.

Check.

diff --git a/doc/src/sgml/standalone-install.xml 
b/doc/src/sgml/standalone-install.xml

index 62582effed..b5d2835a47 100644
--- a/doc/src/sgml/standalone-install.xml
+++ b/doc/src/sgml/standalone-install.xml
@@ -50,7 +50,7 @@ in the stand-alone version.

That one though seems to be what's ending up in INSTALL, according to
the comment at the beginning of the file.


  root# mkdir /usr/local/pgsql/data
  root# chown postgres /usr/local/pgsql/data
  root# su - postgres
-postgres$ /usr/local/pgsql/bin/initdb -D 
/usr/local/pgsql/data
+postgres$ /usr/local/pgsql/bin/pg_ctl -D 
/usr/local/pgsql/data

  

I'm confused here, the paragraph reads "Create a database installation
with the initdb", so I think this hunk is not
correct and should be removed?


That's indeed one replacement too much.

The attached patch is fixing this.



@@ -77,25 +77,21 @@ postgres$ /usr/local/pgsql/bin/initdb 
-D /usr/local/pgsql/data   The previous initdb step should have told 
you how to

   start up the database server. Do so now. The command should look
   something like:
-/usr/local/pgsql/bin/postgres -D 
/usr/local/pgsql/data

- This will start the server in the foreground. To put the server
- in the background use something like:
-nohup /usr/local/pgsql/bin/postgres -D 
/usr/local/pgsql/data \
-    /dev/null server.log 21 /dev/null 

+/usr/local/pgsql/bin/pg_ctl -D 
/usr/local/pgsql/data start

  
    
   To stop a server running in the background you can type:
-kill `cat 
/usr/local/pgsql/data/postmaster.pid`
+/usr/local/pgsql/bin/pg_ctl -D 
/usr/local/pgsql/data stop

  
 
   
  
   Create a database:
-createdb testdb
+/usr/local/pgsql/bin/createdb 
testdb

   Then enter:
-psql testdb
+/usr/local/pgsql/bin/psql 
testdb

   to connect to that database. At the prompt you can enter SQL
   commands and start experimenting.
  

Check.



Thank you for the review!



Submitted this to the Commitfest, and took the liberty to add you as a 
reviewer.



Thanks,

--
Andreas 'ads' Scherbaum
German PostgreSQL User Group
European PostgreSQL User Group - Board of Directors
Volunteer Regional Contact, Germany - PostgreSQL Project

Re: partitioned indexes and tablespaces

[Alvaro Herrera]

On 2018-Nov-03, Tom Lane wrote:

> Hmm ... in the April thread, one of the main concerns that prevented hasty
> action was fear of breaking dump/restore behavior.  Have you checked that
> with this change, a dump/restore will restore the same state (same
> actual tablespace assignments) that existed in the source DB?

I just did, and it does.  The tablespaces are changed with individual
"SET default_tablespace" lines whenever it changes between dumping two
indexes.

> How about if the parent partitioned index's tablespace assignment has
> been changed since a child index was made?

Each index is created independently, with the correct default
tablespace, and then they are all attached together to the parent index
using ALTER INDEX ATTTACH PARTITION.  The tablespace assignments are
identical to the source database.

> What happens with the --no-tablespaces option?

No "SET default_tablespace" lines are emitted in this case, so
everything ends up in the default tablespace, as expected.

> I think I'm okay with this change if the answers to all those questions
> are sane, but I didn't see them discussed in this thread.

I think they are.

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

Re: INSTALL file

[Andreas 'ads' Scherbaum]

On 03.11.18 18:06, Stephen Frost wrote:

Greetings,

* Andreas 'ads' Scherbaum (a...@pgug.de) wrote:

On 02.11.18 01:38, Stephen Frost wrote:

* Andreas 'ads' Scherbaum (a...@pgug.de) wrote:

How about the attached one? Picked up your draft, and cleaned it up a bit.

(unsurprisingly) this is looking pretty good to me.

A few additional notes:

Incorporated. See the attached.

If that works for you, I will submit it to the Commitfest.

Yeah, looks pretty good to me, please register it in the CF system.


Done. Added you for review.

--
Andreas 'ads' Scherbaum
German PostgreSQL User Group
European PostgreSQL User Group - Board of Directors
Volunteer Regional Contact, Germany - PostgreSQL Project

Re: bugfix: BUG #15477: Procedure call with named inout refcursor parameter - "invalid input syntax for type boolean"

[Tom Lane]

Pavel Stehule  writes:
> pá 2. 11. 2018 v 9:02 odesílatel Peter Eisentraut <
> peter.eisentr...@2ndquadrant.com> napsal:
>> Could you explain your analysis of the problem and how this patch
>> proposes to fix it?

> The original code works with original function arguments - and this compare
> with argmodes fields. But when you use named args, this expectation is not
> valid.

Yeah.  IMO, the fundamental mistake in this code was to try to avoid
calling expand_function_arguments.  The proposed patch still tries to
avoid that, which is why it's still a mess and, I suspect, still fails
on args with default values.  You cannot process calls with named args
correctly without accounting for defaults.

I'm going to go see about converting this to just call
expand_function_arguments and then drop all the special-case code.

BTW, it looks to me like ExecuteCallStmt trashes the passed-in CallStmt
in cases where expand_function_arguments is not a no-op.  Is that
really safe?  Seems to me it'd cause problems if, for example, dealing
with a CallStmt that's part of a prepared stmt or cached plan.

regards, tom lane

Re: ToDo: show size of partitioned table

[Pavel Stehule]

pá 2. 11. 2018 v 6:17 odesílatel Amit Langote 
napsal:

> Hi,
>
> On 2018/11/01 2:19, Pavel Stehule wrote:
> > st 31. 10. 2018 v 7:34 odesílatel Amit Langote <
> > langote_amit...@lab.ntt.co.jp> napsal:
> >> On 2018/10/31 15:30, Pavel Stehule wrote:
> >>> st 31. 10. 2018 v 3:27 odesílatel Amit Langote <
> >>> langote_amit...@lab.ntt.co.jp> napsal:
>  +appendPQExpBufferStr(, "\nWHERE c.relkind IN ('p')\n");
> 
>  I wonder if we should list partitioned indexes ('I') as well, because
>  their size information is not available with \di+.  But maybe, they
> >> should
>  have a separate command.
> 
> >>>
> >>> I though about it too and I prefer separate command. Similar to \di+
> >>
> >> Okay, maybe \dI+.
> >>
> >>
> > what about combination
> >
> > \dPt+ for partitined tables and size
> > \dPi+ for indexes on partitioned tables and size
> > \dP+ for total partition size (tables + indexes)
>
> +1
>

here is a patch

postgres=# \dt+
  List of relations
┌┬───┬───┬───┬─┬─┐
│ Schema │   Name│ Type  │ Owner │  Size   │ Description │
╞╪═══╪═══╪═══╪═╪═╡
│ public │ data  │ table │ pavel │ 0 bytes │ │
│ public │ data_2016 │ table │ pavel │ 18 MB   │ │
│ public │ data_2017 │ table │ pavel │ 17 MB   │ │
└┴───┴───┴───┴─┴─┘
(3 rows)

postgres=# \di+
   List of relations
┌┬┬───┬───┬───┬─┬─┐
│ Schema │  Name  │ Type  │ Owner │   Table   │  Size   │
Description │
╞╪╪═══╪═══╪═══╪═╪═╡
│ public │ data_2016_inserted_idx │ index │ pavel │ data_2016 │ 11 MB
│ │
│ public │ data_2016_value_idx│ index │ pavel │ data_2016 │ 15 MB
│ │
│ public │ data_2017_inserted_idx │ index │ pavel │ data_2017 │ 10 MB
│ │
│ public │ data_2017_value_idx│ index │ pavel │ data_2017 │ 14 MB
│ │
│ public │ data_inserted_idx  │ index │ pavel │ data  │ 0 bytes
│ │
│ public │ data_value_idx │ index │ pavel │ data  │ 0 bytes
│ │
└┴┴───┴───┴───┴─┴─┘
(6 rows)

postgres=# \dP+
 List of partitioned relations
┌┬──┬───┬───┬─┐
│ Schema │ Name │ Owner │ Size  │ Description │
╞╪══╪═══╪═══╪═╡
│ public │ data │ pavel │ 84 MB │ │
└┴──┴───┴───┴─┘
(1 row)

postgres=# \dPt+
  List of partitioned tables
┌┬──┬───┬───┬─┐
│ Schema │ Name │ Owner │ Size  │ Description │
╞╪══╪═══╪═══╪═╡
│ public │ data │ pavel │ 35 MB │ │
└┴──┴───┴───┴─┘
(1 row)

postgres=# \dPi+
List of partitioned indexes
┌┬───┬───┬───┬───┬─┐
│ Schema │   Name│ Owner │ Table │ Size  │ Description │
╞╪═══╪═══╪═══╪═══╪═╡
│ public │ data_inserted_idx │ pavel │ data  │ 21 MB │ │
│ public │ data_value_idx│ pavel │ data  │ 28 MB │ │
└┴───┴───┴───┴───┴─┘
(2 rows)


> Thanks,
> Amit
>
>
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index eb9d93a168..fb30571e2c 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1635,6 +1635,49 @@ testdb=
 
   
 
+
+  
+\dP[+] [ pattern ]
+
+
+Lists partitioned relations. If pattern is
+specified, only entries whose relation name or schema name matches
+the pattern are listed.  If the form \dP+
+is used, a sum of size of related partitions and a description
+are also displayed.
+
+
+  
+
+
+  
+\dPi[+] [ pattern ]
+
+
+Lists partitioned indexes. If pattern is
+specified, only entries whose index name or schema name matches
+the pattern are listed.  If the form \dPi+
+is used, a sum of size of related indexes and a description
+are also displayed.
+
+
+  
+
+
+  
+\dPt[+] [ pattern ]
+
+
+Lists partitioned tables. If pattern is
+specified, only entries whose table name or schema name matches
+the pattern are listed.  If the form \dPt+
+is used, a sum of size of related indexes and a description
+are also displayed.
+
+
+  
+
+
   
 \drds [ role-pattern [ database-pattern ] ]
 
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 

Re: plruby: rb_iterate symbol clash with libruby.so

[Tom Lane]

Andres Freund  writes:
> On 2018-11-03 14:39:45 -0400, Tom Lane wrote:
>> Andres Freund  writes:
>>> ISTM this specific case we could solve the issue by opening plruby.so /
>>> extension sos with RTLD_DEEPBIND.  That doesn't work if ruby extensions
>>> that are loaded later use rb_iterate, but should work for the case above.

>> Doesn't work on non-glibc platforms, though.

> Yea, but I'm not sure there's anything portable to do about such cases :/

The portable answer is to rename to avoid the symbol conflict.

>>> I don't mind the precedent that much, but isn't that also not unlikely
>>> to break other extensions that use those functions?

>> I rather doubt there are any.  Also, if there are, the RTLD_DEEPBIND
>> solution would break them too, no?

> Why would it break? Deepbind just means the to-be-opened .so is put
> first in the search path, if there's no match it'll still look in other
> sos.

Yeah, but once plruby is loaded, any subsequently loaded .so has
got two possible ways to resolve rb_iterate.  No matter what we do,
the behavior will be wrong for some of them.

regards, tom lane

Re: partitioned indexes and tablespaces

[Tom Lane]

Alvaro Herrera  writes:
> On 2018-Nov-03, Andrew Dunstan wrote:
>> +1. This is unquestionably a POLA violation that should be fixed, IMNSHO.

> Yeah, that's my view on it too.
> Pushed.

Hmm ... in the April thread, one of the main concerns that prevented hasty
action was fear of breaking dump/restore behavior.  Have you checked that
with this change, a dump/restore will restore the same state (same
actual tablespace assignments) that existed in the source DB?  How about
if the parent partitioned index's tablespace assignment has been changed
since a child index was made?  What happens with the --no-tablespaces
option?

I think I'm okay with this change if the answers to all those questions
are sane, but I didn't see them discussed in this thread.

regards, tom lane

Re: partitioned indexes and tablespaces

[Andres Freund]

On 2018-11-03 16:24:28 -0300, Alvaro Herrera wrote:
> On 2018-Nov-03, Robert Haas wrote:
> > Well, you've guaranteed that already.  Now 11 will be different from
> > 11.1, and tables will be different from indexes until somebody goes
> > and makes that consistent again.
> 
> Nobody is running 11.0 in production.

Uh, I know of people doing so.

Greetings,

Andres Freund

Re: partitioned indexes and tablespaces

[Alvaro Herrera]

On 2018-Nov-03, Robert Haas wrote:

> Well, you've guaranteed that already.  Now 11 will be different from
> 11.1, and tables will be different from indexes until somebody goes
> and makes that consistent again.

Nobody is running 11.0 in production.  The people using 11.0 are just
testing, and those that run into this behavior have already reported as
an inconvenience, not something to rely on.

> I now wish I hadn't objected to changing the behavior in April.  If
> I'd know that the alternative was going to be to change it in
> November, back-patched, two days before a minor release, with more
> people voting against the change than for it, I would have kept my
> mouth shut.

Well, you didn't object to changing index behavior in April.  You
objected to a change related to tables.  Nobody had noticed this
behavior for indexes.

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

Re: plruby: rb_iterate symbol clash with libruby.so

[Andres Freund]

On 2018-11-03 14:39:45 -0400, Tom Lane wrote:
> Andres Freund  writes:
> >> Pavel Raiskup  writes:
> >>> Is it realistic we could rename red-black tree methods from 'rb_*' to e.g.
> >>> 'rbt_*' to avoid this clash?
> 
> > ISTM this specific case we could solve the issue by opening plruby.so /
> > extension sos with RTLD_DEEPBIND.  That doesn't work if ruby extensions
> > that are loaded later use rb_iterate, but should work for the case above.
> 
> Doesn't work on non-glibc platforms, though.

Yea, but I'm not sure there's anything portable to do about such cases :/

> > On 2018-11-03 14:19:46 -0400, Tom Lane wrote:
> >> That's not terribly appetizing, because it essentially means we're giving
> >> Ruby (and potentially every other library on the planet) veto power over
> >> our function namespace.  That does not scale, especially not when the
> >> feedback loop has a time constant measured in years :-(
> >> I don't have a huge objection to renaming the rbtree functions, other
> >> than the precedent it sets ...
> 
> > I don't mind the precedent that much, but isn't that also not unlikely
> > to break other extensions that use those functions?
> 
> I rather doubt there are any.  Also, if there are, the RTLD_DEEPBIND
> solution would break them too, no?

Why would it break? Deepbind just means the to-be-opened .so is put
first in the search path, if there's no match it'll still look in other
sos.

Greetings,

Andres Freund

Re: plruby: rb_iterate symbol clash with libruby.so

[Tom Lane]

Andres Freund  writes:
>> Pavel Raiskup  writes:
>>> Is it realistic we could rename red-black tree methods from 'rb_*' to e.g.
>>> 'rbt_*' to avoid this clash?

> ISTM this specific case we could solve the issue by opening plruby.so /
> extension sos with RTLD_DEEPBIND.  That doesn't work if ruby extensions
> that are loaded later use rb_iterate, but should work for the case above.

Doesn't work on non-glibc platforms, though.

> On 2018-11-03 14:19:46 -0400, Tom Lane wrote:
>> That's not terribly appetizing, because it essentially means we're giving
>> Ruby (and potentially every other library on the planet) veto power over
>> our function namespace.  That does not scale, especially not when the
>> feedback loop has a time constant measured in years :-(
>> I don't have a huge objection to renaming the rbtree functions, other
>> than the precedent it sets ...

> I don't mind the precedent that much, but isn't that also not unlikely
> to break other extensions that use those functions?

I rather doubt there are any.  Also, if there are, the RTLD_DEEPBIND
solution would break them too, no?

regards, tom lane

Re: plruby: rb_iterate symbol clash with libruby.so

[Andres Freund]

Hi,

On 2018-11-03 14:19:46 -0400, Tom Lane wrote:
> Pavel Raiskup  writes:
> > Hi, I'm curious how it worked before (seems like the function is defined
> > in both PostgreSQL and Ruby projects for quite some time) - but I recently
> > came across this situation:
> > - /bin/postgres is build-time linked with 'ld -E'
> > - /bin/postgres dlopen()s plruby.so
> > - plruby.so calls rb_iterate, but linker prefers rb_iterate defined in
> >   /bin/postgres, instead of (the wanted one) rb_iterate from libruby.so
> > This means an ugly PG server crash.  I'm curious what to do with this;
> > ideally it would be solvable from plruby.so itself, but there doesn't seem
> > to be nice solution (one could do some hacks with dlopen('libruby.so')).
> 
> Bleah.

ISTM this specific case we could solve the issue by opening plruby.so /
extension sos with RTLD_DEEPBIND.  That doesn't work if ruby extensions
that are loaded later use rb_iterate, but should work for the case above.


> We recently noticed that using a --version-script symbol filter on shared
> libraries fixes some cases of this problem, because a non-exported symbol
> will be preferentially resolved inside the library.  I guess that's of no
> use for this particular case though, since evidently Ruby has to export
> its rb_iterate for Ruby extensions to use.

I wondered every now and then if we shouldn't use dlmopen to open
extension objects, when available. If we were to do it right we'd create
a namespace for every shared object, use dlmopen to expose postgres'
symbols, and then load the extension objects in separate namespaces.
But I think that's not feasible, because:
"The glibc implementation supports a maximum of 16 namespaces."


> > Is it realistic we could rename red-black tree methods from 'rb_*' to e.g.
> > 'rbt_*' to avoid this clash?
> 
> That's not terribly appetizing, because it essentially means we're giving
> Ruby (and potentially every other library on the planet) veto power over
> our function namespace.  That does not scale, especially not when the
> feedback loop has a time constant measured in years :-(
> 
> I don't have a huge objection to renaming the rbtree functions, other
> than the precedent it sets ...

I don't mind the precedent that much, but isn't that also not unlikely
to break other extensions that use those functions?

Greetings,

Andres Freund

Re: partitioned indexes and tablespaces

[Robert Haas]

On Fri, Nov 2, 2018 at 7:12 PM Alvaro Herrera  wrote:
> With all due respect, this argument makes no sense.  All partitioned
> indexes that exist today have a null reltablespace (all pg_class rows
> already have a reltablespace column; I'm not changing that).  If users

I hope not, because that column isn't nullable.

> want to keep the current behavior (i.e. that indexes on future
> partitions are created in the default tablespace), all they have to do
> is not send any ALTER INDEX changing the index's tablespace.
>
> You're saying that people currently running Postgres 11 that are
> doing "CREATE INDEX ... TABLESPACE foo" will be surprised because the
> index ends up in tablespace foo for partitions they create afterwards.
>
> Additionally, you're saying that all people currently doing
> ALTER INDEX ... SET TABLESPACE foo
> expect that
> 1. the parent partitioned index silently does not change at all
> 2. the indexes on future partitions end up in the default tablespace.
>
> I cannot see how that's for real.
>
> Furthermore, I think delaying this change to pg12 serves nobody, because
> it just means that there will be a behavior difference for no reason.

Well, you've guaranteed that already.  Now 11 will be different from
11.1, and tables will be different from indexes until somebody goes
and makes that consistent again.

I now wish I hadn't objected to changing the behavior in April.  If
I'd know that the alternative was going to be to change it in
November, back-patched, two days before a minor release, with more
people voting against the change than for it, I would have kept my
mouth shut.

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

Re: plruby: rb_iterate symbol clash with libruby.so

[Tom Lane]

Pavel Raiskup  writes:
> Hi, I'm curious how it worked before (seems like the function is defined
> in both PostgreSQL and Ruby projects for quite some time) - but I recently
> came across this situation:
> - /bin/postgres is build-time linked with 'ld -E'
> - /bin/postgres dlopen()s plruby.so
> - plruby.so calls rb_iterate, but linker prefers rb_iterate defined in
>   /bin/postgres, instead of (the wanted one) rb_iterate from libruby.so
> This means an ugly PG server crash.  I'm curious what to do with this;
> ideally it would be solvable from plruby.so itself, but there doesn't seem
> to be nice solution (one could do some hacks with dlopen('libruby.so')).

Bleah.

We recently noticed that using a --version-script symbol filter on shared
libraries fixes some cases of this problem, because a non-exported symbol
will be preferentially resolved inside the library.  I guess that's of no
use for this particular case though, since evidently Ruby has to export
its rb_iterate for Ruby extensions to use.

> Is it realistic we could rename red-black tree methods from 'rb_*' to e.g.
> 'rbt_*' to avoid this clash?

That's not terribly appetizing, because it essentially means we're giving
Ruby (and potentially every other library on the planet) veto power over
our function namespace.  That does not scale, especially not when the
feedback loop has a time constant measured in years :-(

I don't have a huge objection to renaming the rbtree functions, other
than the precedent it sets ...

regards, tom lane

Re: WIP Patch: Add a function that returns binary JSONB as a bytea

[Merlin Moncure]

On Fri, Nov 2, 2018 at 5:15 PM Andrew Dunstan
 wrote:
> On 11/02/2018 05:20 PM, Andres Freund wrote:
> > Hi,
> >
> > On 2018-11-02 17:02:24 -0400, Andrew Dunstan wrote:
> >> On 11/02/2018 11:34 AM, Merlin Moncure wrote:
> >>> Binary format consuming applications already have to deal with these
> >>> kinds of issues. We already expose internal structures in the other
> >>> functions -- not sure why jsonb is held to a different standard.  For
> >>> other data types where format changes were made, the standard of
> >>> 'caveat version' was in place to protect the user.  For jsonb we
> >>> decided to implement a version flag within the type itself, which I
> >>> thought mistake at the time -- better to have a version header in the
> >>> COPY BINARY if needed.
> >>>
> >>
> >> jsonb_send does output a version header, as I pointed out upthread.
> > That's Merlin's point I think. For reasons I don't quite understand he
> > doesn't like that. Yes, a global solution would have been prettier than
> > per-datum version flag, but that obvioulsy wasn't realistic to introduce
> > around the feature freeze of the version that introduced jsonb.
>
>
> Oh, hmm. It would have been a big change of little value, ISTM. One byte
> of overhead per jsonb datum for a version indicator doesn't seem a huge
> price to pay.

Yeah -- it really isn't.

My concern was more that it seemed odd to protect one type with a
version flag where others aren't; format agreement strikes me as more
of a protocol negotiation thing than an aspect of each individual data
point.  It also makes for slightly odd (IMO) client side coding. The
contract is of concern, not the overhead.

It works well enough though...we discussed this a bit when the header
was introduced and the discussion ended exactly the same way :-).

merlin

Re: INSTALL file

[Stephen Frost]

Greetings,

* Andreas 'ads' Scherbaum (a...@pgug.de) wrote:
> On 02.11.18 01:38, Stephen Frost wrote:
> >* Andreas 'ads' Scherbaum (a...@pgug.de) wrote:
> >>How about the attached one? Picked up your draft, and cleaned it up a bit.
> >(unsurprisingly) this is looking pretty good to me.
> >
> >A few additional notes:
> 
> Incorporated. See the attached.
> 
> If that works for you, I will submit it to the Commitfest.

Yeah, looks pretty good to me, please register it in the CF system.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: partitioned indexes and tablespaces

[Alvaro Herrera]

On 2018-Nov-03, Andrew Dunstan wrote:

> +1. This is unquestionably a POLA violation that should be fixed, IMNSHO.

Yeah, that's my view on it too.

Pushed.

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

Re: Special role for subscriptions

[Stephen Frost]

Greetings,

* Evgeniy Efimkin (efim...@yandex-team.ru) wrote:
> In postgresql 10 and 11 only superuser can create/alter subscriptions.
> If there was a special role (like pg_monitor), it would be more easy to grant 
> control on subscriptions.
> I can make a patch if there are no objections against it.

I think the short answer is 'yes, we should let non-superusers do that',
but the longer answer is:

What level of access makes sense for managing subscriptions?  Should
there be a way to say "user X is allowed to create a subscription for
remote system Y, but only for tables that exist in schema Q"?

My general feeling is 'yes', though, of course, I don't want to say that
we have to have all of that before we move forward with allowing
non-superusers to create subscriptions, but I do think we want to make
sure that we have a well thought-out path for how to get from where we
are now to a system which has a lot more granularity, and to do our best
to try avoiding any paths that might paint us into a corner.

Thanks!

Stephen


signature.asc
Description: PGP signature

plruby: rb_iterate symbol clash with libruby.so

[Pavel Raiskup]

Hi, I'm curious how it worked before (seems like the function is defined
in both PostgreSQL and Ruby projects for quite some time) - but I recently
came across this situation:

- /bin/postgres is build-time linked with 'ld -E'
- /bin/postgres dlopen()s plruby.so
- plruby.so calls rb_iterate, but linker prefers rb_iterate defined in
  /bin/postgres, instead of (the wanted one) rb_iterate from libruby.so

This means an ugly PG server crash.  I'm curious what to do with this;
ideally it would be solvable from plruby.so itself, but there doesn't seem
to be nice solution (one could do some hacks with dlopen('libruby.so')).

Is it realistic we could rename red-black tree methods from 'rb_*' to e.g.
'rbt_*' to avoid this clash?

Pavel

RE: Internal error XX000 with enable_partition_pruning=on, pg 11 beta1 on Debian

[Phil Florent]

Hi,
Thanks for your work, our prototype runs OK. PostgreSQL 11 and its now fully 
functional partitioning feature is our validated choice to replace a well-known 
proprietary RDBMS in 100+ public hospitals for our dss application.
Best regards
Phil


De : Amit Langote 
Envoyé : jeudi 9 août 2018 06:35
À : Tom Lane
Cc : David Rowley; Rushabh Lathia; Alvaro Herrera; Robert Haas; Phil Florent; 
PostgreSQL Hackers
Objet : Re: Internal error XX000 with enable_partition_pruning=on, pg 11 beta1 
on Debian

On 2018/08/09 13:00, Tom Lane wrote:
> Amit Langote  writes:
>> One reason why we should adapt such a test case is that, in the future, we
>> may arrange for make_partitionedrel_pruneinfo(), whose code we just fixed,
>> to not be called if we know that run-time pruning is not needed.  It seems
>> that that's true for the test added by the commit, that is, it doesn't
>> need run-time pruning.
>
> Not following your argument here.  Isn't make_partition_pruneinfo
> precisely what is in charge of figuring out whether run-time pruning
> is possible?

With the current coding, yes, it is...

> (See my point in the other thread about Jaime's assertion crash,
> that no run-time pruning actually would be possible for that query.
> But we got to the assertion failure anyway.)

The first time I'd seen that make_partition_pruneinfo *always* gets called
from create_append_plan if rel->baserestrictinfo is non-NIL, I had
wondered whether we couldn't avoid doing it for the cases for which we'll
end up throwing away all that work anyway.  But looking at the code now,
it may be a bit hard -- analyze_partkey_exprs(), which determines whether
we'll need any execution-time pruning, could not be called any sooner.

So, okay, I have to admit that my quoted argument isn't that strong.

Thanks,
Amit

Re: COPY FROM WHEN condition

[Daniel Verite]

David Fetter wrote:

> It also seems like a violation of separation of concerns to couple
> FEBE to grammar, so there'd need to be some way to do those things
> separately, too.

After re-reading psql/copy.c, I withdraw what I said upthread:
it doesn't appear necessary to add anything to support the WHEN
condition with \copy.

\copy does have a dedicated mini-parser, but it doesn't try to
recognize every option: it's only concerned with getting the bits of
information that are needed to perform the client-side work:
- whether it's a copy from or to
- what exact form and value has the 'filename' argument immediately
 after from or to:
 '' | PROGRAM '' | stdin | stdout | pstdout | pstdout

It doesn't really care what the options are, just where they are
in the buffer, so they can be copied into the COPY SQL statement.

From the code:
 * The documented syntax is:
 *  \copy tablename [(columnlist)] from|to filename [options]
 *  \copy ( query stmt ) to filename [options]

The WHEN clause would be part of the [options], which
are handled as simply as this in parse_slash_copy():

  /* Collect the rest of the line (COPY options) */
  token = strtokx(NULL, "", NULL, NULL,
  0, false, false, pset.encoding);
  if (token)
  result->after_tofrom = pg_strdup(token);

So unless there's something particular in the WHEN clause
expression that could make this strtokx() invocation error out,
this should work directly.

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

Re: partitioned indexes and tablespaces

[Andrew Dunstan]

On 11/02/2018 07:12 PM, Alvaro Herrera wrote:

On 2018-Nov-03, Michael Paquier wrote:


On Fri, Nov 02, 2018 at 03:53:51PM -0300, Alvaro Herrera wrote:

In this thread I'm not proposing to change the behavior for tables, only
for indexes.  If people want to change behavior for tables (and I agree
with doing so), they can start their own threads.

Changing this behavior on back-branches is not a good idea I think
because that changes the representation of the pg_class entries in the
catalogs by adding the reltablespace reference for a partitioned index
which does not exist now.  We are long past the time of doing such
changes on v11, but that can perfectly be done for v12.

With all due respect, this argument makes no sense.  All partitioned
indexes that exist today have a null reltablespace (all pg_class rows
already have a reltablespace column; I'm not changing that).  If users
want to keep the current behavior (i.e. that indexes on future
partitions are created in the default tablespace), all they have to do
is not send any ALTER INDEX changing the index's tablespace.

You're saying that people currently running Postgres 11 that are
doing "CREATE INDEX ... TABLESPACE foo" will be surprised because the
index ends up in tablespace foo for partitions they create afterwards.

Additionally, you're saying that all people currently doing
ALTER INDEX ... SET TABLESPACE foo
expect that
1. the parent partitioned index silently does not change at all
2. the indexes on future partitions end up in the default tablespace.

I cannot see how that's for real.

Furthermore, I think delaying this change to pg12 serves nobody, because
it just means that there will be a behavior difference for no reason.




+1. This is unquestionably a POLA violation that should be fixed, IMNSHO.

cheers

andrew

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

Re: pgbench -M option can be specified more than once

[Tatsuo Ishii]

> Andres Freund  writes:
>> On 2018-11-03 10:12:14 +0900, Tatsuo Ishii wrote:
>>> One of my colleagues actually believed that if both "-M extended" and
>>> "-M prepared" were specified, pgbench runs in mixture of those
>>> modes. So I felt avoiding such misunderstanding is more important.
> 
>> I regularly specify options multiple times, and I'd hate for that to not
>> work anymore.  It's really useful to set defaults for scripts that then
>> can be overwritten by the caller by passing on "$@" (i.e. all script
>> arguments).
> 
> Yeah, there's an ancient tradition that "last switch wins" when receiving
> conflicting command-line options.  I think we should tread very carefully
> in breaking that.

Yes, I think that is a fair argument. Proposal withdrawn.

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

Re: pread() and pwrite()

[Thomas Munro]

On Sat, Nov 3, 2018 at 2:07 AM Jesper Pedersen
 wrote:
> This still applies, and passes make check-world.
>
> I wonder what the commit policy is on this, if the Windows part isn't
> included. I read Heikki's comment [1] as it would be ok to commit
> benefiting all platforms that has pread/pwrite.

Here's a patch to add Windows support by supplying
src/backend/port/win32/pread.c.  Thoughts?

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


0001-Use-pread-pwrite-instead-of-lseek-read-write-v8.patch
Description: Binary data


0002-Supply-pread-pwrite-implementations-for-Windows-v8.patch
Description: Binary data

Re: Constraint documentation

[Fabien COELHO]

Thanks for your remarks and advices, and of course for your help to
rewrite the text.
So, it is now included in the new version attached.
I hope it will be ok this time.


At least it looks ok to me.

Patch applies cleanly, doc build ok.

I've put the patch as "Ready".

--
Fabien.

Re: New function pg_stat_statements_reset_query() to reset statistics of a specific query

[Amit Kapila]

On Fri, Sep 28, 2018 at 7:45 AM Haribabu Kommi  wrote:
>
> On Tue, Sep 25, 2018 at 3:09 PM Michael Paquier  wrote:
>>
>> On Tue, Sep 25, 2018 at 01:49:09PM +1000, Haribabu Kommi wrote:
>> Hmm.  I see a problem with the tests and the stability of what
>> pg_stat_statements_reset() can return.  Normally installcheck is
>> disabled in contrib/pg_stat_statements/Makefile but if you remove this
>> barrier and run the tests with a server loading the module in
>> shared_preload_libraries then things are not stable.  We don't have this
>> kind of instability on HEAD.  Some call to pg_stat_statements_reset()
>> system-wide is visibly missing.
>
>
> The difference in results of the output of the pg_stat_statements_reset()
> function is based on the earlier statements that are stored in the 
> pg_stat_statements
> table, this varies based on the environment. So I created a wrapper function
> that masks the return value of the first reset and then the test is stable.
>
> check whether is it fine or any better approach to handle it?
>

Before trying out any solution or deciding which is better, I think we
want to understand why the variability in results occurred only after
your patch?  Without the patch, it works just fine.

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

Re: pgbench doc fix

[Fabien COELHO]

"prepared: use extended query protocol with reused prepared statements"


I don't think this mouthful is useful in the --help output.  The
existing wording gets the message across just fine, I think.  More
details can be put in the reference page.


These suggestions are for the online doc page, and possibly an internal 
comment in the code.


The pgbench --help just states the 3 possible values and the say which is 
the default, and indeed cannot say much more.


--
Fabien.

Re: zheap storage_engine parameter, shouldn't this raise an error?

[Amit Kapila]

On Sat, Nov 3, 2018 at 3:21 PM Amit Kapila  wrote:
>
> On Sat, Nov 3, 2018 at 3:13 PM Daniel Westermann 
>  wrote:
>>
>> Hi,
>>
>>
>> I believe this should raise an error or at least a warning?
>>
>
> Sure, if we want we can raise the error or warning for this, but this is a 
> parameter mainly to test zheap with existing set of regression tests.  I am 
> not sure if we want to keep it or even if we want to have any such parameter 
> for testing purpose in what form it will be present.  So, adding more checks 
> at this stage around this parameter doesn't seem advisable to me.
>

Instead of starting new threads for each report, it might be better to
report it on the zheap thread [1].

[1] - 
https://www.postgresql.org/message-id/CAA4eK1%2BYtM5vxzSM2NZm%2BpC37MCwyvtkmJrO_yRBQeZDp9Wa2w%40mail.gmail.com

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

Re: zheap storage_engine parameter, shouldn't this raise an error?

[Amit Kapila]

On Sat, Nov 3, 2018 at 3:13 PM Daniel Westermann <
daniel.westerm...@dbi-services.com> wrote:

> Hi,
>
>
> I believe this should raise an error or at least a warning?
>
>
> postgres=# alter system set storage_engine = 'zheap';
> ALTER SYSTEM
> postgres=# alter system set storage_engine = 'zheap2';
> ALTER SYSTEM
> postgres=# alter system set storage_engine = 'zheap3';
> ALTER SYSTEM
> postgres=#
>
>
Sure, if we want we can raise the error or warning for this, but this is a
parameter mainly to test zheap with existing set of regression tests.  I am
not sure if we want to keep it or even if we want to have any such
parameter for testing purpose in what form it will be present.  So, adding
more checks at this stage around this parameter doesn't seem advisable to
me.

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

zheap storage_engine parameter, shouldn't this raise an error?

[Daniel Westermann]

Hi,


I believe this should raise an error or at least a warning?


postgres=# alter system set storage_engine = 'zheap';
ALTER SYSTEM
postgres=# alter system set storage_engine = 'zheap2';
ALTER SYSTEM
postgres=# alter system set storage_engine = 'zheap3';
ALTER SYSTEM
postgres=#

Regards
Daniel




[cid:9bfad55c-fe48-4c76-8ec8-8b344f9b4094]
Daniel Westermann | Delivery Manager & Open Infrastructure Technology Leader
Phone: +41 32 422 96 00 | Mobile: +41 79 927 24 46 | Fax: +41 32 422 96 15
dbi services, Voltastrasse 104 | CH-4056 Basel
daniel.westerm...@dbi-services.com
www.dbi-services.com

Re: pgbench doc fix

[Peter Eisentraut]

On 03/11/2018 01:08, Tatsuo Ishii wrote:
> I like this. But maybe we can remove "named"?
> 
> "prepared: use extended query protocol with reused prepared statements"

I don't think this mouthful is useful in the --help output.  The
existing wording gets the message across just fine, I think.  More
details can be put in the reference page.

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

Re: csv format for psql

[Fabien COELHO]

Bonjour Daniel,


Here's a rebased version with a couple regression tests
added per the discussions during the previous CF.

Now at https://commitfest.postgresql.org/20/1861/


Patch applies cleanly, compiles, make check ok, doc gen ok.

Fine with me. I switched the patch to "Ready".

--
Fabien.

Re: Hooks to Modify Execution Flow and Query Planner

[Vincent Mirian]

Hi Amit,

Thank you for your response. Chapters 51, 57 and 59 (Overview of PostgreSQL
Internals, Writing A Foreign Data Wrapper and Writing A Custom Scan
Provider) seem to be relevant. Aside from the source code snippets in the
document, is there functional source code that can be used as reference?
Also, aside from this mailing list, is there an interactive medium for
asking questions?

Thank you,

On Thu, Nov 1, 2018 at 1:57 AM Amit Langote 
wrote:

> Hi,
>
> On 2018/11/01 16:58, Vincent Mirian wrote:
> > Hi all,
> >
> > I would like to create a library with UDFs written in C that implements
> > different Query Planner tasks (e.g. scan, hash, join, etc...). I am
> looking
> > for a document that provides an overview of execution flow within
> postgres
> > and the query planner. I am also looking for a description of the
> software
> > data structures and interfaces used.
>
> Maybe, the following chapter in Postgres documentation will help as a
> start:
>
> https://www.postgresql.org/docs/11/static/overview.html
>
> For studying internal data structures and interfaces, you can also read
> the comments contained in the source code and README files containing
> descriptions of various data structures and interfaces, which is a often
> recommended method.
>
> Specifically, if you just want to inject alternative plan nodes for the
> individual scan, hash, join operators needed to compute a query, but want
> the Postgres query planner to take care of the whole planning itself, you
> might consider looking into the Custom Scan Provider facility:
>
> https://www.postgresql.org/docs/current/static/custom-scan.html
>
> With it, you can write C code that gets invoked at certain points during
> planning and execution, where you can add your special/custom node to a
> plan and do execution related tasks on those nodes, respectively.  With
> this approach, Postgres planner and executor take care of most of the
> details of planning and execution, whereas your code implements the
> specialized logic you developed for, say, scanning a disk file, joining
> two or more tables, building a hash table from the data read from a table,
> etc.
>
> You can alternatively formulate your code as a foreign data wrapper if all
> you want do is model a non-Postgres data source as regular Postgres tables.
>
> https://www.postgresql.org/docs/11/static/fdwhandler.html
>
>
> If you don't intend to add new plan nodes or define a new type of foreign
> table, but want to alter the planning or execution itself (or parts
> thereof), you may want to look at various planner and executor hooks.  For
> example, if you want to replace the whole planner, which takes a parsed
> query (the Query struct) and returns a plan (the PlannedStmt struct whose
> internals you'll need to figure out if your alternative planning code can
> produce a valid one), you can use the following hook:
>
> /* Hook for plugins to get control in planner() */
> typedef PlannedStmt *(*planner_hook_type) (Query *parse,
>int cursorOptions,
>ParamListInfo boundParams);
>
> But that may be too complex a hook to implement on your own, so you can
> look at more granular hooks which allow certain points within the
> planning, such as:
>
> /* Hook for plugins to get control in set_rel_pathlist() */
> typedef void (*set_rel_pathlist_hook_type) (PlannerInfo *root,
> RelOptInfo *rel,
> Index rti,
> RangeTblEntry *rte);
>
> /* Hook for plugins to get control in add_paths_to_joinrel() */
> typedef void (*set_join_pathlist_hook_type) (PlannerInfo *root,
>  RelOptInfo *joinrel,
>  RelOptInfo *outerrel,
>  RelOptInfo *innerrel,
>  JoinType jointype,
>  JoinPathExtraData *extra);
>
> /* Hook for plugins to replace standard_join_search() */
> typedef RelOptInfo *(*join_search_hook_type) (PlannerInfo *root,
>   int levels_needed,
>   List *initial_rels);
>
> /* Hook for plugins to get control in get_relation_info() */
> typedef void (*get_relation_info_hook_type) (PlannerInfo *root,
>  Oid relationObjectId,
>  bool inhparent,
>  RelOptInfo *rel);
>
> On the executor side, you got:
>
> /* Hook for plugins to get control in ExecutorStart() */
> typedef void (*ExecutorStart_hook_type) (QueryDesc *queryDesc, int eflags);
>
> /* Hook for plugins to get control in ExecutorRun() */
> typedef void (*ExecutorRun_hook_type) (QueryDesc *queryDesc,
>  

Re: pgbench -M option can be specified more than once

[Fabien COELHO]

Hello Tatsuo-san,


While playing with pgbench, I found multiple "-M query_mode" can be
set more than once.


As already said by others, the "last one win" is somehow a useful feature, 
so I'd prefer avoiding erroring out on this one.


This could leave:

(1) improving pgbench doc by spelling out the last one win behavior, but 
this is rather standard ;


(2) only warning when this accurs, but that would become noisy for 
legitimate uses ;


(3) doing nothing about it.

--
Fabien.