Re: [HACKERS] taking stdbool.h into use

[Tom Lane]

Andres Freund  writes:
> On March 20, 2018 8:24:41 PM PDT, Michael Paquier  wrote:
>> Yeah, I agree with that.  Just not using stdbool.h in those cases ought
>> to be fine.  Any platforms where sizeof(bool) is 4 involves macos older
>> than 10.5 and Windows platforms using MSVC versions older than 2003
>> (didn't look further down either).

> Aren't there some somewhat modern architectures where that's still the case, 
> for performance reasons? PPC or such?

Well, hydra (F16 on ppc64) has sizeof(bool) = 1.  Don't have any other
good datapoints handy.  Presumably we'd set up configure to report
what it found out, so it wouldn't take long to survey the buildfarm.

regards, tom lane

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Tom Lane]

Michael Paquier  writes:
> On Tue, Mar 20, 2018 at 01:27:35PM -0400, Tom Lane wrote:
>> +   if (flags & GUC_LIST_QUOTE)
>> +   elog(FATAL, "extensions cannot define GUC_LIST_QUOTE 
>> variables");

> This would be better as an ereport with ERRCODE_FEATURE_NOT_SUPPORTED I
> think.  An ERROR is better in my opinion.

I don't mind making it an ereport, but I think it needs to be FATAL
for the reason stated in the comment.

>> +   record = find_option(name, false, WARNING);
>> +   if (record == NULL)

> LOG instead of WARNING?

I did it like that to match the similar code in flatten_set_variable_args.
In the end it doesn't matter, because find_option only uses its elevel
argument when create_placeholders is passed as true.

regards, tom lane

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Michael Paquier]

On Tue, Mar 20, 2018 at 01:27:35PM -0400, Tom Lane wrote:
> 1. Only GUC_LIST_QUOTE variables need to be special-cased.  It works
> fine to treat plain LIST variables (e.g. datestyle) with the regular
> code path.  This is good because there are so few of them.

Check.

> 2. We should make an effort to minimize the number of places that know
> explicitly which variables are GUC_LIST_QUOTE.  In the backend, the
> right thing to do is ask guc.c.  In pg_dump, there's no convenient
> way to ask the backend (and certainly nothing that would work against
> old servers), but we should at least make sure there's only one place
> to fix not two.

The only way I can think about for that would be to provide this
information in pg_settings using a text[] array or such which lists all
the GUC flags of a parameter.  That's a hell lot of infrastructure
though which can be easily replaced with the maintenance of a small
hardcoded parameter list.

> 3. We need to prevent extensions from trying to create GUC_LIST_QUOTE
> variables, because those are not going to work correctly if they're
> set before the extension is loaded, nor is there any hope of pg_dump
> dumping them correctly.

This really depends on the elements of the list involved here, so
pg_dump may be able to dump them correctly, though not reliably as that
would be fully application-dependent.  At the end I think that I am on
board with what you are proposing here.

> The attached 0001 patch does the first two things, and could be
> back-patched.  The 0002 patch, which I'd only propose for HEAD,
> is one way we could address point 3.  A different way would be
> to throw a WARNING rather than ERROR and then just mask off the
> problematic bit.  Or we could decide that either of these cures
> is worse than the disease, but I don't really agree with that.

Looks roughly sane to me.

+   if (flags & GUC_LIST_QUOTE)
+   elog(FATAL, "extensions cannot define GUC_LIST_QUOTE
variables");
This would be better as an ereport with ERRCODE_FEATURE_NOT_SUPPORTED I
think.  An ERROR is better in my opinion.

-if (pg_strcasecmp(configitem, "DateStyle") == 0
-|| pg_strcasecmp(configitem, "search_path") == 0)
+if (GetConfigOptionFlags(configitem, true) & GUC_LIST_QUOTE)
For boolean-based comparisons, I would recommend using a comparison with
zero.

+   record = find_option(name, false, WARNING);
+   if (record == NULL)
LOG instead of WARNING?
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] taking stdbool.h into use

[Andres Freund]

On March 20, 2018 8:24:41 PM PDT, Michael Paquier  wrote:
>On Tue, Mar 20, 2018 at 02:14:23PM -0700, Andres Freund wrote:
>> On 2018-03-20 17:04:22 -0400, Peter Eisentraut wrote:
>> > On 3/20/18 02:18, Tom Lane wrote:
>> > > I think it'd be worth identifying exactly which platforms have
>> > > sizeof(bool) different from 1.  Are any of them things that
>anyone
>> > > cares about going forward?  The point of this patch is to ease
>> > > future development of extensions, but it's unlikely any extension
>> > > authors care about portability onto, say, macOS 10.4
>(prairiedog).
>> > 
>> > I'm not sure I follow.  Say we commit configure tests and discover
>that
>> > platforms A, B, and C are affected.  What would we do with that
>> > information?  I don't think we are saying we'd just break A, B, and
>C.
>> 
>> If those are only older platforms we could just not use stdbool for
>> those. The likelihood of getting into conflicts with $library stdbool
>> uses is lower...
>
>Yeah, I agree with that.  Just not using stdbool.h in those cases ought
>to be fine.  Any platforms where sizeof(bool) is 4 involves macos older
>than 10.5 and Windows platforms using MSVC versions older than 2003
>(didn't look further down either).

Aren't there some somewhat modern architectures where that's still the case, 
for performance reasons? PPC or such?

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

Re: Removing useless DISTINCT clauses

[Melanie Plageman]

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

This is a review of the patch to remove useless DISTINCT columns from the 
DISTINCT clause
https://www.postgresql.org/message-id/CAKJS1f8UMJ137sRuVSnEMDDpa57Q71JuLZi4yLCFMekNYVYqaQ%40mail.gmail.com


Contents & Purpose
==

This patch removes any additional columns in the DISTINCT clause when the
table's primary key columns are entirely present in the DISTINCT clause. This
optimization works because the PK columns functionally determine the other
columns in the relation. The patch includes regression test cases.

Initial Run
===
The patch applies cleanly to HEAD. All tests which are run as part of the
`installcheck` target pass correctly (all existing tests pass, all new tests
pass with the patch applied and fail without it). All TAP tests pass. All tests
which are run as part of the `installcheck-world` target pass except one of the
Bloom contrib module tests in `contrib/bloom/sql/bloom.sql`,
 `CREATE INDEX bloomidx ON tst USING bloom (i, t) WITH (col1 = 3);`
which is currently also failing (crashing) for me on master, so it is unrelated 
to the
patch. The test cases seem to cover the new behavior.

Functionality
=
Given that this optimization is safe for the GROUP BY clause (you can remove
functionally dependent attributes from the clause there as well), the logic
does seem to follow for DISTINCT. It seems semantically correct to do this. As
for the implementation, the author seems to have reasonably addressed concerns
expressed over the distinction between DISTINCT and DISTINCT ON. As far as I
can see, this should be fine.

Code Review
===
For a small performance hit but an improvement in readability, the length check
can be moved from the individual group by and distinct clause checks into the
helper function

if (list_length(parse->distinctClause) < 2)
  return;

and

if (list_length(parse->groupClause) < 2)
  return;

can be moved into `remove_functionally_dependent_clauses`


Comments/Documentation
===

The main helper function that is added `remove_functionally_dependent_clauses`
uses one style of comment--with the name of the function and then the rest of
the description indented which is found some places in the code, 
/*
 * remove_functionally_dependent_clauses
 *  Processes clauselist and removes any items which are deemed to 
be
 *  functionally dependent on other clauselist items.
 *
 * If any item from the list can be removed, then a new list is built which
 * does not contain the removed items.  If no item can be removed then the
 * original list is returned.
 */

while other helper functions in the same file use a different style--all lines
flush to the side with a space. I'm not sure which is preferred

/*
 * limit_needed - do we actually need a Limit plan node?
 *
 * If we have constant-zero OFFSET and constant-null LIMIT, we can skip adding
 * a Limit node.  This is worth checking for because "OFFSET 0" is a common
 * locution for an optimization fence.  (Because other places in the planner
 ...

it looks like the non-indented ones are from older commits (2013 vs 2016).

The list length change is totally optional, but I'll leave it as Waiting On 
Author in case the author wants to make that change.

Overall, LGTM.

The new status of this patch is: Waiting on Author

Re: [HACKERS] taking stdbool.h into use

[Michael Paquier]

On Tue, Mar 20, 2018 at 02:14:23PM -0700, Andres Freund wrote:
> On 2018-03-20 17:04:22 -0400, Peter Eisentraut wrote:
> > On 3/20/18 02:18, Tom Lane wrote:
> > > I think it'd be worth identifying exactly which platforms have
> > > sizeof(bool) different from 1.  Are any of them things that anyone
> > > cares about going forward?  The point of this patch is to ease
> > > future development of extensions, but it's unlikely any extension
> > > authors care about portability onto, say, macOS 10.4 (prairiedog).
> > 
> > I'm not sure I follow.  Say we commit configure tests and discover that
> > platforms A, B, and C are affected.  What would we do with that
> > information?  I don't think we are saying we'd just break A, B, and C.
> 
> If those are only older platforms we could just not use stdbool for
> those. The likelihood of getting into conflicts with $library stdbool
> uses is lower...

Yeah, I agree with that.  Just not using stdbool.h in those cases ought
to be fine.  Any platforms where sizeof(bool) is 4 involves macos older
than 10.5 and Windows platforms using MSVC versions older than 2003
(didn't look further down either).
--
Michael


signature.asc
Description: PGP signature

Re: PATCH: Configurable file mode mask

[Michael Paquier]

On Tue, Mar 20, 2018 at 05:44:22PM -0400, Stephen Frost wrote:
> * David Steele (da...@pgmasters.net) wrote:
>> On 3/16/18 11:12 AM, Stephen Frost wrote:
>> It seems to me that pg_basebackup and pg_receivexlog should have a -g
>> option to control the mode of the files that they write to disk (not
>> including the modes stored in the tar files).
>> 
>> Or perhaps we should just update the perms in the tar files for now and
>> leave the rest alone.
> 
> Having options to pg_basebackup to control what's done makes sense to
> me- but whatever those options do, I'd expect them to apply equally to
> the tar files and to the files extracted with plain mode.  Having those
> be different really strikes me as very odd.

Agreed for the consistency part, permissions should be applied
consistently for the folder and the tar format.

Having the option for pg_receivewal definitely makes sense to me, as it
is the one in charge of opening and writing the WAL segments.  For
pg_basebackup, let's not forget that there is one tar file for each
tablespace, and that each file is received separately using a COPY
stream.  There is some logic already which parses the tar header part of
an individual file in order to look for recovery.conf (see
ReceiveTarFile() in pg_basebackup.c).  It would be possible to enforce
grouping permissions when receiving each file, and this would be rather
low-cost in performance I think.  Honestly, my vote would go for having
the permissions set correctly by the source server as this brings
consistency to the whole experience without complicating the interface 
of pg_basebackup, and this also makes the footprint of this patch on
pg_basebackup way lighter.
--
Michael


signature.asc
Description: PGP signature

Re: JIT compiling with LLVM v12.2

[Andres Freund]

Hi,

On 2018-03-20 23:03:13 -0400, Stephen Frost wrote:
> Greetings,
> 
> * Andres Freund (and...@anarazel.de) wrote:
> > On 2018-03-20 19:29:55 -0700, Andres Freund wrote:
> > > On 2018-03-21 15:22:08 +1300, Thomas Munro wrote:
> > > > Somehow your configure test correctly concludes that my $CC (clang
> > > > 4.0) doesn't support -fexcess-precision=standard but that my $CXX
> > > > (clang++ 4.0) does, despite producing a nearly identical warning:
> > > 
> > > Yea, there was a copy & pasto (s/ac_c_werror_flag/ac_cxx_werror_flag/),
> > > sorry. If you rebase onto the committed version, it should work?  I'll
> > > push a version rebased of the jit tree soon.
> > 
> > Well, or not. Seems git.pg.o is down atm:
> > 
> > debug1: Next authentication method: publickey
> > debug1: Offering public key: RSA 
> > SHA256:cMbSa8YBm8AgaIeMtCSFvvPDrrrdadCxzQaFiWFe+7c /home/andres/.ssh/id_rsa
> > debug1: Server accepts key: pkalg ssh-rsa blen 277
> > 
> > 
> > Will try tomorrow.
> 
> Andres contacted pginfra over IRC about this, but it seems that it
> resolved itself shortly following (per a comment from Andres to that
> effect), so, afaik, things are working properly.

Indeed. I've pushed a rebased version now, that basically just fixes the
issue Thomas observed.

Thanks,

Andres Freund

Re: JIT compiling with LLVM v12.2

[Stephen Frost]

Greetings,

* Andres Freund (and...@anarazel.de) wrote:
> On 2018-03-20 19:29:55 -0700, Andres Freund wrote:
> > On 2018-03-21 15:22:08 +1300, Thomas Munro wrote:
> > > Somehow your configure test correctly concludes that my $CC (clang
> > > 4.0) doesn't support -fexcess-precision=standard but that my $CXX
> > > (clang++ 4.0) does, despite producing a nearly identical warning:
> > 
> > Yea, there was a copy & pasto (s/ac_c_werror_flag/ac_cxx_werror_flag/),
> > sorry. If you rebase onto the committed version, it should work?  I'll
> > push a version rebased of the jit tree soon.
> 
> Well, or not. Seems git.pg.o is down atm:
> 
> debug1: Next authentication method: publickey
> debug1: Offering public key: RSA 
> SHA256:cMbSa8YBm8AgaIeMtCSFvvPDrrrdadCxzQaFiWFe+7c /home/andres/.ssh/id_rsa
> debug1: Server accepts key: pkalg ssh-rsa blen 277
> 
> 
> Will try tomorrow.

Andres contacted pginfra over IRC about this, but it seems that it
resolved itself shortly following (per a comment from Andres to that
effect), so, afaik, things are working properly.

If anyone has issues with git.p.o, please let us know, but hopefully all
is good now.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: JIT compiling with LLVM v12.2

[Andres Freund]

On 2018-03-20 19:29:55 -0700, Andres Freund wrote:
> Hi,
> 
> On 2018-03-21 15:22:08 +1300, Thomas Munro wrote:
> > Somehow your configure test correctly concludes that my $CC (clang
> > 4.0) doesn't support -fexcess-precision=standard but that my $CXX
> > (clang++ 4.0) does, despite producing a nearly identical warning:
> 
> Yea, there was a copy & pasto (s/ac_c_werror_flag/ac_cxx_werror_flag/),
> sorry. If you rebase onto the committed version, it should work?  I'll
> push a version rebased of the jit tree soon.

Well, or not. Seems git.pg.o is down atm:

debug1: Next authentication method: publickey
debug1: Offering public key: RSA 
SHA256:cMbSa8YBm8AgaIeMtCSFvvPDrrrdadCxzQaFiWFe+7c /home/andres/.ssh/id_rsa
debug1: Server accepts key: pkalg ssh-rsa blen 277


Will try tomorrow.

Greetings,

Andres Freund

Re: JIT compiling with LLVM v12.2

[Andres Freund]

Hi,

On 2018-03-21 15:22:08 +1300, Thomas Munro wrote:
> Somehow your configure test correctly concludes that my $CC (clang
> 4.0) doesn't support -fexcess-precision=standard but that my $CXX
> (clang++ 4.0) does, despite producing a nearly identical warning:

Yea, there was a copy & pasto (s/ac_c_werror_flag/ac_cxx_werror_flag/),
sorry. If you rebase onto the committed version, it should work?  I'll
push a version rebased of the jit tree soon.

Greetings,

Andres Freund

Re: JIT compiling with LLVM v12.2

[Thomas Munro]

On Wed, Mar 21, 2018 at 1:50 PM, Andres Freund  wrote:
> On 2018-03-20 03:14:55 -0700, Andres Freund wrote:
>> My current plan is to push the first few commits relatively soon, give
>> the BF a few cycles to shake out. Set up a few BF animals with each
>> supported LLVM version. Then continue mergin.
>
> I've done that.  I'll set up a number of BF animals as soon as I've got
> the buildfarm secrets for them.

Somehow your configure test correctly concludes that my $CC (clang
4.0) doesn't support -fexcess-precision=standard but that my $CXX
(clang++ 4.0) does, despite producing a nearly identical warning:

configure:5489: checking whether ccache cc supports
-fexcess-precision=standard, for CFLAGS
configure:5511: ccache cc -c -Wall -Wmissing-prototypes
-Wpointer-arith -Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
-fwrapv -fexcess-precision=standard  conftest.c >&5
cc: warning: optimization flag '-fexcess-precision=standard' is not
supported [-Wignored-optimization-argument]
configure:5511: $? = 0
configure: failed program was:
...
configure:5521: result: no

configure:5528: checking whether ccache c++ supports
-fexcess-precision=standard, for CXXFLAGS
configure:5556: ccache c++ -c -Wall -Wpointer-arith
-fno-strict-aliasing -fwrapv -fexcess-precision=standard  conftest.cpp
>&5
c++: warning: optimization flag '-fexcess-precision=standard' is not
supported [-Wignored-optimization-argument]
configure:5556: $? = 0
configure:5572: result: yes

So it goes into my $CXXFLAGS and then I get the same warning when
compiling the three .cpp files in the tree.

GCC also doesn't like that in C++ mode, but it seems to report an
error (rather than a warning) so with g++ as your $CXX configure sees
$? = 1 and draws the correct conclusion.

$ gcc -fexcess-precision=standard -c test.c
$ g++ -fexcess-precision=standard -c test.cpp
cc1plus: sorry, unimplemented: -fexcess-precision=standard for C++

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

Re: Question about WalSndWriteData

[Peter Eisentraut]

On 3/16/18 12:08, Konstantin Knizhnik wrote:
> pq_putmessage_noblock copies data from ctx->out buffer to libpq buffers.
> After it we write timestamp to ctx->out buffer.
> And comments says that we should do it "as late as possible".
> But this timestamp is not included in the copy data packet which is 
> already copied to libpq connection buffer.

There is a pq_flush_if_writable() right after this that will write out
the rest of ctx->out.

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

Re: JIT compiling with LLVM v12.2

[Andres Freund]

Hi,

On 2018-03-20 03:14:55 -0700, Andres Freund wrote:
> My current plan is to push the first few commits relatively soon, give
> the BF a few cycles to shake out. Set up a few BF animals with each
> supported LLVM version. Then continue mergin.

I've done that.  I'll set up a number of BF animals as soon as I've got
the buildfarm secrets for them.

- Andres

Re: WIP: BRIN multi-range indexes

[Tomas Vondra]

On 03/04/2018 01:14 AM, Tomas Vondra wrote:
> ...
> 
> The one overflow issue I found in the patch is that the numeric
> "distance" function does this:
> 
> d = DirectFunctionCall2(numeric_sub, a2, a1); /* a2 - a1 */
> 
> PG_RETURN_FLOAT8(DirectFunctionCall1(numeric_float8, d));
> 
> which can overflow, of course. But that is not fatal - the index may get
> inefficient due to non-optimal merging of ranges, but it will still
> return correct results. But I think this can be easily improved by
> passing not only the two values, but also minimum and maximum, and use
> that to normalize the values to [0,1].
> 

Attached is an updated patch series, addressing this possible overflow
the way I proposed - by computing (a2 - a1) / (b2 - b1), which is
guaranteed to produce a value between 0 and 1.

The two new arguments are ignored for most "distance" functions, because
those can't overflow or underflow in double precision AFAICS.

regards

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


0001-Pass-all-keys-to-BRIN-consistent-function-a-20180320.patch.gz
Description: application/gzip


0002-Move-IS-NOT-NULL-checks-to-bringetbitmap-20180320.patch.gz
Description: application/gzip


0003-BRIN-bloom-indexes-20180320.patch.gz
Description: application/gzip


0004-BRIN-multi-range-minmax-indexes-20180320.patch.gz
Description: application/gzip

Re: [HACKERS] Optional message to user when terminating/cancelling backend

[Daniel Gustafsson]

> On 20 Mar 2018, at 12:12, Eren Başak  wrote:

> I reviewed the patch. Here are my notes:

Thanks!

> The patch does not add new tests (maybe isolation tests) for the new feature. 
> Are there any opportunities to have tests for confirming the new behavior?

Good point, not sure why I’ve forgotten to add this.  No existing suite seemed
to match well, so I added a new one for system administration functions like
this one.

> Not introduced with this patch, but the spacing between the functions is not 
> consistent in src/backend/storage/ipc/backend_signal.c. There are 2 newlines 
> after some functions (like pg_cancel_backend_msg) and 1 newline after others 
> (like pg_terminate_backend_msg). It would be nice to fix these while 
> refactoring.

Fixed.

> Another thing is that, in a similar manner, we could allow changing the error 
> code which might be useful for extensions. For example, Citus could use it to 
> cancel remote backends when it detects a distributed deadlock and changes the 
> error code to something retryable while doing so. For reference, see the hack 
> that Citus is currently using: 
> https://github.com/citusdata/citus/blob/81cbb7c223f2eb9cfa8903f1d28869b6f972ded1/src/backend/distributed/shared_library_init.c#L237

In 20170620200134.ubnv4sked5seo...@alap3.anarazel.de, Andres suggested the same
thing.  I don’t disagree, but I’m also not sure what the API would look like so
I’d prefer to address that in a follow-up patch should this one get accepted.

> +If the optional message parameter is set, the text
> +will be appended to the error message returned to the signalled backend.
> 
> I think providing more detail would be useful. For example we can provide an 
> example about how the error message looks like in its final form or what will 
> happen if the message is too long.

Expanded the documentation a little and added a (contrived) example.

> -pg_signal_backend(int pid, int sig)
> +pg_signal_backend(int pid, int sig, char *msg)
> 
> The new parameter (msg) is not mentioned in the function header comment. This 
> applies to pg_signal_backend, pg_cancel_backend_internal, 
> pg_terminate_backend_internal functions.

Fixed.

> +Datum
> +pg_cancel_backend(PG_FUNCTION_ARGS)
> +{
> + PG_RETURN_BOOL(pg_cancel_backend_internal(PG_GETARG_INT32(0), NULL));
> +}
> +
> +Datum
> +pg_cancel_backend_msg(PG_FUNCTION_ARGS)
> +{
> + pid_t   pid;
> + char   *msg = NULL;
> +
> + if (PG_ARGISNULL(0))
> + ereport(ERROR,
> + (errmsg("pid cannot be NULL")));
> +
> + pid = PG_GETARG_INT32(0);
> +
> + if (PG_NARGS() == 2 && !PG_ARGISNULL(1))
> + msg = text_to_cstring(PG_GETARG_TEXT_PP(1));
> +
> + PG_RETURN_BOOL(pg_cancel_backend_internal(pid, msg));
> +}
> 
> The first function seem redundant here because the second one covers all the 
> cases.

pg_cancel_backend() is defined proisstrict, while pg_cancel_backend_msg() is
not.  I think we must be able to perform the cancellation if the message is
NULL, so made it two functions.

Thinking more about this, I think pg_cancel_backend_msg() should handle a NULL
pid in the same way as pg_cancel_backend() so fixed that as well.

> + memset(slot->message, '\0', sizeof(slot->message));
> 
> SetBackendCancelMessage uses memset while BackendCancelShmemInit uses MemSet. 
> Not a big deal but it would be nice to be consistent and use postgres macro 
> versions for such calls. 

Good point, moved to MemSet() for both.

> +int
> +SetBackendCancelMessage(pid_t backend, char *message)
> +{
> + BackendCancelShmemStruct *slot;
> 
> The variable "slot" is declared outside of the foor loop but only used in the 
> inside, therefore it would be nicer to declare it inside the loop.

Moved to inside the loop.

> + BackendCancelInit(MyBackendId);
> 
> Is the "normal multiuser case" the best place to initialize cancellation? For 
> example, can't we cancel background workers? If this is the right place, 
> maybe we should justify why this is the best place to initialize backend 
> cancellation memory part with a comment.

I didn’t envision this being in another setting than the multiuser case, but
it’s been clear throughout the thread that others have had good ideas around
extended uses of this.  Background workers can still be terminated/canceled,
this only affects setting the message, and that is to me a multiuser feature.

Renamed the functions BackendCancelMessage*() for clarity.

> + char   *buffer = palloc0(MAX_CANCEL_MSG);
> 
> Why not use a static array like char[MAX_CANCEL_MSG], instead of pallocing?

No specific reason, changed.

> +/*
> + * Return the configured cancellation message and its length. If the returned
> + * length is greater than the size of the passed buffer, truncation has been
> + * performed. The message is cleared on reading.
> + */
> +int
> +GetCancelMessage(char **buffer, 

Re: PQHost() undefined behavior if connecting string contains both host and hostaddr types

[Haribabu Kommi]

On Wed, Mar 21, 2018 at 6:06 AM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 3/16/18 00:03, Kyotaro HORIGUCHI wrote:
> > I agree to the conclusion that PQhost() shouldn't return hostaddr
> > "if it has any host name to return". But I still haven't found
> > the reason for returning '/tmp' for IP connection.
> >
> > The attached patch is revised version of that in the following thread.
>
> That patch looks good to me.
>
> Moreover, I wonder whether we shouldn't remove the branch where
> conn->connhost is NULL.  When would that be the case?  The current
> behavior is to sometimes return the actual host connected to, and
> sometimes the host list.  That doesn't make sense.
>

Scenarios where the connection is not yet established, in that scenario
the PQhost() can return the provided connection host information.

Other than the above, it always returns the proper host details.


> I think we should also revert 64f86fb11e20b55fb742af72d55806f8bdd9cd2d,
> in which psql's \conninfo digs out the raw hostaddr value to display,
> which could contain a list of things.  I think \conninfo should just
> display the value returned by PQhost(), and if we change PQhost() to
> take hostaddr into account, then this should address the original
> complaint.


As per my understanding of the commit 64f86fb11e20b55fb742af72d55806
f8bdd9cd2d,
the hostaddr is given the preference while displaying instead of host.

Even with the above PQhost() patch, If user provides both host and hostaddr
as options,
the PQhost() function returns host and not the hostaddr. In
commit 7b02ba62, the support
of "Allow multiple hostaddrs to go with multiple hostnames".

If it is fine to display the host in combination of both host and hostaddr,
then it is fine
remove the commit 64f86fb11e20b55fb742af72d55806f8bdd9cd2d.

Regards,
Hari Babu
Fujitsu Australia

Re: [PoC PATCH] Parallel dump to /dev/null

[Stephen Frost]

Greetings,

* Christoph Berg (christoph.b...@credativ.de) wrote:
> Re: Tom Lane 2018-03-20 <12960.1521557...@sss.pgh.pa.us>
> > It might help if the patch were less enthusiastic about trying to
> > "optimize" by avoiding extra file opens/closes in the case of output
> > to /dev/null.  That seems to account for a lot of the additional
> > complication, and I can't see a reason that it'd be worth it.
> 
> Note that the last patch was just a PoC to check if the extra
> open/close could be avoided. The "real" patch is the 2nd last.

Even so, I'm really not a fan of this patch either.  If we could do this
in a general way where we supported parallel mode with output to stdout
or to a file and then that file could happen to be /dev/null, I'd be
more interested because it's at least reasonable for someone to want
that beyond using pg_dump to (poorly) check for corruption.

As it is, this is an extremely special case which may even end up being
confusing for users (I can run a parallel pg_dump to /dev/null, but not
to a regular file?!).

Instead of trying to use pg_dump for this, we should provide a way to
actually check for corruption across everything (instead of just the
heap..), and have all detected corruption reported in one pass.

One of the things that I really like about PostgreSQL is that we
typically try to provide appropriate tools for the job and avoid just
hacking things to give users a half-solution, which is what this seems
like to me.  Let's write a proper tool (perhaps as a background
worker..?) to scan the database (all of it...) which will find and
report corruption anywhere.  That'll take another release to do, but
hopefully pushing back on this will encourage that to happen, whereas
allowing this in would actively discourage someone from writing a proper
tool and we would be much worse off for that.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] [FEATURE PATCH] pg_stat_statements with plans (v02)

[Julian Markwort]

I just realized I made a whitespace error when I modified the comments.
I hope I don't make a habit of sending erroneous mails.

Please accept my apologies, as well as a guaranteed whitespace-error-free patch 
(updated to version 5 for clarity).

Julian

Julian Markwort schrieb am 2018-03-20:
> To anyone who followed along with this for so long, I'd like to present my 
> newest version of this patch.

> As suggested by Arthur Zakirov, there is now only a single GUC ( 
> pg_stat_statements.plan_track ) responsible for the selection of the plans 
> that should be tracked. Possible values are: all, none, good, or bad.
> I've mostly copied functionality from pl_handler.c . This resulted in the 
> need to include varlena.h so I could use the SplitIdentifierString() function 
> to parse the values, of which several (e.g. 
> pg_stat_statements.plan_track='good, bad') could be used.

> I've also added a new GUC:
> pg_stat_statements.plan_fence_factor
> This GUC can be used to scale the fences of the interval, outside of which a 
> plan might be updated.
> Right now, it is set to 1.5 (common factor for the definition of outliers in 
> boxplots) and you can see through additional colums in the pg_stat_statements 
> view, how often these fences are surpassed by execution times and how often 
> the plans are updated. (The colums are: good_plan_outliers, 
> good_plan_updates, bad_plan_outliers, bad_plan_updates and are primarily here 
> for testing and review purposes and are not essential to this patch, they 
> probably don't add any value for the average user)

> Similarly to the first suggestion by Arthur, I've also changed the plan_reset 
> functionality - there is now only one function, 
> pg_stat_statements_plan_reset(queryid bigint), overloaded with (queryid 
> bigint, plantype cstring) args, that can be used to remove both plans (when 
> omitting the cstring) or either of them. The cstring argument accepts 'good' 
> or 'bad'.

> I also added more comments to the estimations of the quartiles and the 
> calculation of the fences.

> The performance impact lies now at 139312 vs 141841 tps, so roughly 1.78% 
> slower than default pg_stat_statements.
> The fact that these results are a little worse than the previous iteration is 
> due to some changes in the definition of the fences which mistakenly 
> calculated by adding the scaled interquartile distance to the mean, instead 
> of adding it to the respective quartiles, which means that plan updates are 
> triggered a little more often.
> For 4259631 transactions however, only 11 updates for the bad plans where 
> triggered.



> I'm looking forward to your opinions!
> Julian
diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index 39b368b70e..49bb462d10 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -4,7 +4,8 @@ MODULE_big = pg_stat_statements
 OBJS = pg_stat_statements.o $(WIN32RES)
 
 EXTENSION = pg_stat_statements
-DATA = pg_stat_statements--1.4.sql pg_stat_statements--1.4--1.5.sql \
+DATA = pg_stat_statements--1.4.sql \
+	pg_stat_statements--1.5--1.6.sql pg_stat_statements--1.4--1.5.sql \
 	pg_stat_statements--1.3--1.4.sql pg_stat_statements--1.2--1.3.sql \
 	pg_stat_statements--1.1--1.2.sql pg_stat_statements--1.0--1.1.sql \
 	pg_stat_statements--unpackaged--1.0.sql
diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index 5318c3550c..2ca549686f 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -395,4 +395,40 @@ SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
  SELECT pg_stat_statements_reset() | 1 |1
 (8 rows)
 
+-- test to see if any plans have been recorded.
+SELECT
+  CASE WHEN good_plan_time > 0 THEN 1 ELSE 0 END,
+  CASE WHEN bad_plan_time > 0 THEN 1 ELSE 0 END,
+  CASE WHEN good_plan_timestamp >= timestamp '1970-01-01 00:00:00' THEN 1 ELSE 0 END,
+  CASE WHEN bad_plan_timestamp >= timestamp '1970-01-01 00:00:00' THEN 1 ELSE 0 END
+FROM pg_stat_statements ORDER BY query COLLATE "C";
+ case | case | case | case 
+--+--+--+--
+0 |0 |0 |0
+0 |0 |0 |0
+0 |0 |0 |0
+0 |0 |0 |0
+0 |0 |0 |0
+0 |0 |0 |0
+1 |1 |1 |1
+1 |1 |1 |1
+1 |1 |1 |1
+(9 rows)
+
+-- test if there is some text in the recorded plans.
+SELECT substr(good_plan, 0, 11), substr(bad_plan, 0, 11) FROM pg_stat_statements ORDER BY query COLLATE "C";
+   substr   |   substr   
++
+| 
+| 
+| 
+| 
+| 
+| 
+ Query Text | Query Text
+ Query Text | Query Text
+ Query Text | Query Text
+ Query Text | Query Text
+(10 rows)
+
 DROP EXTENSION pg_stat_statements;

Re: PATCH: Configurable file mode mask

[Stephen Frost]

David,

* David Steele (da...@pgmasters.net) wrote:
> On 3/16/18 11:12 AM, Stephen Frost wrote:
> > 
> >>> Visibly there would be no need for a -g switch in
> >>> pg_basebackup as it is possible to guess from the received untar'ed
> >>> files what should be the permissions of the data based on what is
> >>> received in pg_basebackup.c.  It would also be necessary to change the
> >>> permissions of pg_wal as this is created before receiving any files.
> >>
> >> This part might be trickier.
> > 
> > This seems like another case where what we should be doing, and what
> > people will be expecting, I'd think, is just what they're used to tar
> > doing in these cases- which would be setting the dir/file mode for each
> > file based on what's in the tarball.  Again, the files which are in the
> > data dir are, sadly, not always just those that PG is familiar with.
> 
> I've been working on this and have become convinced that adding group
> permissions to files that pg_basebackup writes to disk based on whether
> group permissions are enabled in PGDATA isn't the right way to go.
> 
> To be clear, I'm not taking about the permissions set within the tar
> file - I think it makes sense to use the actual PGDATA permissions in
> that case.

What that implies then, if I'm following, is that the results of a
pg_basebackup -Ft && tar -xvf ; would be different from the results of a
pg_basebackup -Fp ; .

That seems like it would be rather odd and confusing to users and so I
have a hard time agreeing that such an inconsistency makes sense.

> pg_basebackup may not be running as postgres, and even if it is I don't
> think we can assume that group access is appropriate for the files that
> it writes.  It's a different environment and different security rules
> may apply.

Sure, and the same security rules may also apply.  Consider an
environment which is managed through a change management system (as many
are these days...) where the pg_basebackup is being run specifically to
set up a replica (which is quite commonly done..) and then allow a tool
like pgbackrest to use both the primary and the replica for backups,
where pgbackrest is run as an independent user which shares the same
group as the PG user and needs group-read access on the replica.  After
building such a replica, the user would have to do a chmod across the
entire data directory, even though the primary was initialized with
group-read access, or, oddly, perform the pg_basebackup to tar files and
then extract those tar files instead of using the plain format.

The general pg_basebackup->replica process works great today and the
primary and the replica more-or-less match as if they were both initdb'd
the same way, or a backup/restore was done, and not preserving the
privileges as they exist would end up diverging from that.

In these cases we're really talking about the defaults here; as I
mention below, I agree that having the ability to control what ends up
happening definitely makes sense (as tar does...).

> It seems to me that pg_basebackup and pg_receivexlog should have a -g
> option to control the mode of the files that they write to disk (not
> including the modes stored in the tar files).
> 
> Or perhaps we should just update the perms in the tar files for now and
> leave the rest alone.

Having options to pg_basebackup to control what's done makes sense to
me- but whatever those options do, I'd expect them to apply equally to
the tar files and to the files extracted with plain mode.  Having those
be different really strikes me as very odd.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] path toward faster partition pruning

[Robert Haas]

On Tue, Mar 20, 2018 at 7:07 AM, Amit Langote
 wrote:
> On 2018/03/16 21:55, Amit Langote wrote:
>> Attached updated patches.
>
> Attached is further revised version.
>
> Of note is getting rid of PartitionPruneContext usage in the static
> functions of partprune.c.  Most of the code there ought to only run during
> planning, so it can access the necessary information from RelOptInfo
> directly instead of copying it to PartitionPruneContext and then passing
> it around.

+if (rte->relkind == RELKIND_PARTITIONED_TABLE)
+{
+if (rel->baserestrictinfo != NIL)
+{
+live_children = prune_append_rel_partitions(root, rel);
+did_pruning = true;
+}
+}

Use &&

+case COMBINE_OR:
+{

Won't survive pgindent, which currently produces a *massive* diff for
these patches.

+/*
+ * XXX- The following ad-hoc method of pruning only works for list
+ * partitioning.  It checks for each partition if all of its
+ * accepted values appear in ne_datums[].
+ */

So why are we doing it this way?  How about doing something not
ad-hoc?  I tried to propose that before.

+ *  Set *value to the constant value obtained by evaluating 'expr'
+ *
+ * Note that we may not be able to evaluate the input expression, in which
+ * case, the function returns false to indicate that *value has not been
+ * set.  True is returned otherwise.

These comments need updating, since this function (laudibly) no longer
does any evaluating.  I wonder how this will work for run-time
pruning, though.

+if (context->partopcintype[partkeyidx] != exprTyp)
+{
+Oid new_supfuncid;
+int16   procnum;
+
+
+procnum = (context->strategy == PARTITION_STRATEGY_HASH)
+? HASHEXTENDED_PROC
+: BTORDER_PROC;
+new_supfuncid = get_opfamily_proc(context->partopfamily[partkeyidx],
+  context->partopcintype[partkeyidx],
+  exprTyp, procnum);
+fmgr_info(new_supfuncid, >partsupfunc[partkeyidx]);
+}

What's the point of this, exactly?  Leftover dead code, maybe?

+ * Input:
+ *  See the comments above the definition of PartScanKeyInfo to see what
+ *  kind of information is contained in 'keys'.

There's no such thing as PartScanKeyInfo any more and the function has
no argument called 'keys'.  None of the functions actual arguments are
explained.

+/*
+ * If there are multiple pruning steps, we perform them one after another,
+ * passing the result of one step as input to another.  Based on the type
+ * of pruning step, perform_pruning_step may add or remove partitions from
+ * the set of partitions it receives as the input.
+ */

The comment sounds great, but the code doesn't work that way; it
always calls bms_int_members to intersect the new result with any
previous result.  I'm baffled as to how this manages to DTRT if
COMBINE_OR is used.  In general I had hoped that the list of pruning
steps was something over which we were only going to iterate, not
recurse.  This definitely recurses for the combine steps, but it's
still (sorta) got the idea of a list of iterable steps.  That's a
weird mix.

+if (nvalues == context->partnatts)
+{
+greatest_modulus = get_greatest_modulus(boundinfo);
+rowHash = compute_hash_value(partnatts, partsupfunc, values,
+ isnull);
+result_index = partindices[rowHash % greatest_modulus];
+if (result_index >= 0)
+return bms_make_singleton(result_index);
+}
+else
+/* Can't do pruning otherwise, so return all partitions. */
+return bms_add_range(NULL, 0, context->nparts - 1);

Wouldn't we want to (1) arrange things so that this function is never
called if nvalues < context->partnatts && context->strategy ==
PARTITION_STRATEGY_HASH or at least (2) avoid constructing isnull from
nullkeys if we're not going to use it?

Also, shouldn't we be sanity-checking the strategy number here?

I'm out of time for right now but it looks to me like this patch still
needs quite a bit of fine-tuning.

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

Re: INOUT parameters in procedures

[Merlin Moncure]

On Tue, Mar 20, 2018 at 4:19 PM, Merlin Moncure  wrote:
> A) you can't assign output variables with into:
> CALL p(1) INTO i;  // gives syntax error
>
> B) you can't assign via assignment
> i := p(1); // gives error, 'use CALL'
>
> C) but you *can* via execute
> EXECUTE 'CALL p(1)' INTO i;  // this works!
>
> ...I'm glad 'C' works, as without that there would be no useful way to
> get values out of procedures called from within other
> procedures/functions as things stand today.  'A' ideally also out to
> work, but I'm not sure  'B' should be expected to work since it's
> really a thin layer around SELECT.   What do you think?

Also (sorry for spam),
A procedure created via:
create procedure p() as $$begin call p(); end; $$ language plpgsql;
...will segfault when called -- there ought to be a stack depth check.

merlin

Re: [HACKERS] [FEATURE PATCH] pg_stat_statements with plans (v02)

[Julian Markwort]

To anyone who followed along with this for so long, I'd like to present my 
newest version of this patch.

As suggested by Arthur Zakirov, there is now only a single GUC ( 
pg_stat_statements.plan_track ) responsible for the selection of the plans that 
should be tracked. Possible values are: all, none, good, or bad.
I've mostly copied functionality from pl_handler.c . This resulted in the need 
to include varlena.h so I could use the SplitIdentifierString() function to 
parse the values, of which several (e.g. pg_stat_statements.plan_track='good, 
bad') could be used.

I've also added a new GUC:
pg_stat_statements.plan_fence_factor
This GUC can be used to scale the fences of the interval, outside of which a 
plan might be updated.
Right now, it is set to 1.5 (common factor for the definition of outliers in 
boxplots) and you can see through additional colums in the pg_stat_statements 
view, how often these fences are surpassed by execution times and how often the 
plans are updated. (The colums are: good_plan_outliers, good_plan_updates, 
bad_plan_outliers, bad_plan_updates and are primarily here for testing and 
review purposes and are not essential to this patch, they probably don't add 
any value for the average user)

Similarly to the first suggestion by Arthur, I've also changed the plan_reset 
functionality - there is now only one function, 
pg_stat_statements_plan_reset(queryid bigint), overloaded with (queryid bigint, 
plantype cstring) args, that can be used to remove both plans (when omitting 
the cstring) or either of them. The cstring argument accepts 'good' or 'bad'.

I also added more comments to the estimations of the quartiles and the 
calculation of the fences.

The performance impact lies now at 139312 vs 141841 tps, so roughly 1.78% 
slower than default pg_stat_statements.
The fact that these results are a little worse than the previous iteration is 
due to some changes in the definition of the fences which mistakenly calculated 
by adding the scaled interquartile distance to the mean, instead of adding it 
to the respective quartiles, which means that plan updates are triggered a 
little more often.
For 4259631 transactions however, only 11 updates for the bad plans where 
triggered.



I'm looking forward to your opinions!
Julian
diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index 39b368b70e..49bb462d10 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -4,7 +4,8 @@ MODULE_big = pg_stat_statements
 OBJS = pg_stat_statements.o $(WIN32RES)
 
 EXTENSION = pg_stat_statements
-DATA = pg_stat_statements--1.4.sql pg_stat_statements--1.4--1.5.sql \
+DATA = pg_stat_statements--1.4.sql \
+	pg_stat_statements--1.5--1.6.sql pg_stat_statements--1.4--1.5.sql \
 	pg_stat_statements--1.3--1.4.sql pg_stat_statements--1.2--1.3.sql \
 	pg_stat_statements--1.1--1.2.sql pg_stat_statements--1.0--1.1.sql \
 	pg_stat_statements--unpackaged--1.0.sql
diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index 5318c3550c..2ca549686f 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -395,4 +395,40 @@ SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
  SELECT pg_stat_statements_reset() | 1 |1
 (8 rows)
 
+-- test to see if any plans have been recorded.
+SELECT
+  CASE WHEN good_plan_time > 0 THEN 1 ELSE 0 END,
+  CASE WHEN bad_plan_time > 0 THEN 1 ELSE 0 END,
+  CASE WHEN good_plan_timestamp >= timestamp '1970-01-01 00:00:00' THEN 1 ELSE 0 END,
+  CASE WHEN bad_plan_timestamp >= timestamp '1970-01-01 00:00:00' THEN 1 ELSE 0 END
+FROM pg_stat_statements ORDER BY query COLLATE "C";
+ case | case | case | case 
+--+--+--+--
+0 |0 |0 |0
+0 |0 |0 |0
+0 |0 |0 |0
+0 |0 |0 |0
+0 |0 |0 |0
+0 |0 |0 |0
+1 |1 |1 |1
+1 |1 |1 |1
+1 |1 |1 |1
+(9 rows)
+
+-- test if there is some text in the recorded plans.
+SELECT substr(good_plan, 0, 11), substr(bad_plan, 0, 11) FROM pg_stat_statements ORDER BY query COLLATE "C";
+   substr   |   substr   
++
+| 
+| 
+| 
+| 
+| 
+| 
+ Query Text | Query Text
+ Query Text | Query Text
+ Query Text | Query Text
+ Query Text | Query Text
+(10 rows)
+
 DROP EXTENSION pg_stat_statements;
diff --git a/contrib/pg_stat_statements/pg_stat_statements--1.5--1.6.sql b/contrib/pg_stat_statements/pg_stat_statements--1.5--1.6.sql
new file mode 100644
index 00..6c8f743ee5
--- /dev/null
+++ b/contrib/pg_stat_statements/pg_stat_statements--1.5--1.6.sql
@@ -0,0 +1,82 @@
+/* contrib/pg_stat_statements/pg_stat_statements--1.5--1.6.sql */
+
+-- complain if script 

Re: INOUT parameters in procedures

[Merlin Moncure]

On Tue, Mar 20, 2018 at 10:09 AM, Pavel Stehule  wrote:
> 2018-03-20 15:18 GMT+01:00 Merlin Moncure :
>> >> postgres=# create or replace procedure p(a inout int default 7) as $$
>> >> begin return; end; $$ language plpgsql;
>> >> CREATE PROCEDURE
>> >> Time: 1.182 ms
>> >> postgres=# call p();
>> >>  a
>> >> ───
>> >>  0
>> >> (1 row)
>> >
>> >
>> > I wrote patch
>>
>> Confirmed this fixes the issue.
>
> Thanks for info

You're welcome.  Working with this feature some more, I noticed that:
A) you can't assign output variables with into:
CALL p(1) INTO i;  // gives syntax error

B) you can't assign via assignment
i := p(1); // gives error, 'use CALL'

C) but you *can* via execute
EXECUTE 'CALL p(1)' INTO i;  // this works!

...I'm glad 'C' works, as without that there would be no useful way to
get values out of procedures called from within other
procedures/functions as things stand today.  'A' ideally also out to
work, but I'm not sure  'B' should be expected to work since it's
really a thin layer around SELECT.   What do you think?

merlin

Re: [HACKERS] taking stdbool.h into use

[Peter Eisentraut]

On 3/20/18 02:18, Tom Lane wrote:
> I think it'd be worth identifying exactly which platforms have
> sizeof(bool) different from 1.  Are any of them things that anyone
> cares about going forward?  The point of this patch is to ease
> future development of extensions, but it's unlikely any extension
> authors care about portability onto, say, macOS 10.4 (prairiedog).

I'm not sure I follow.  Say we commit configure tests and discover that
platforms A, B, and C are affected.  What would we do with that
information?  I don't think we are saying we'd just break A, B, and C.

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

Add "docs" to top-level Makefile for non-GNU make?

[Thomas Munro]

Hi hackers,

I regularly type "make docs" *doh* "gmake docs" on one system and
"gmake docs" *doh* "make docs" on another.  Is there any reason we
shouldn't add it, for consistency with other targets like check,
installcheck etc?  Patch attached.

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


0001-Add-docs-to-top-level-Makefile-for-non-GNU-make.patch
Description: Binary data

Re: [HACKERS] Partition-wise aggregation/grouping

[Robert Haas]

On Tue, Mar 20, 2018 at 10:46 AM, Jeevan Chalke
 wrote:
> I have added all these three patches in the attached patch-set and rebased
> my changes over it.
>
> However, I have not yet made this patch-set dependednt on UPPERREL_TLIST
> changes you have proposed on another mail-thread and thus it has 0001 patch
> refactoring the scanjoin issue.
> 0002, 0003 and 0004 are your patches added in this patchset.
> 0005 and 0006 are further refactoring patches. 0006 adds a
> GroupPathExtraData which stores mostly child variant data and costs.
> 0007 is main partitionwise aggregation patch which is then rebased
> accordingly.
> 0008 contains testcase and 0009 contains FDW changes.

Committed my refactoring patches (your 0002-0004).

Regarding apply_scanjoin_target_to_paths in 0001 and 0007, it seems
like what happens is: we first build an Append path for the topmost
scan/join rel.  That uses paths from the individual relations that
don't necessarily produce the final scan/join target.  Then we mutate
those relations in place during partition-wise aggregate so that they
now do produce the final scan/join target and generate some more paths
using the results.  So there's an ordering dependency, and the same
pathlist represents different things at different times.  That is, I
suppose, not technically any worse than what we're doing for the
scan/join rel's pathlist in general, but here there's the additional
complexity that the paths get used both before and after being
mutated.  The UPPERREL_TLIST proposal would clean this up, although I
realize that has unresolved issues.

In create_partial_grouping_paths, the loop that does "for (i = 0; i <
2; i++)" is not exactly what I had in mind when I said that we should
use two loops.  I did not mean a loop with two iterations.  I meant
adding a loop like foreach(lc, input_rel->pathlist) in each place
where we currently have a loop like
foreach(input_rel->partial_pathlist).  See 0001, attached.

Don't write if (a) Assert(b) but rather Assert(!a || b).  See 0002, attached.

In the patch as proposed, create_partial_grouping_paths() can get
called even if GROUPING_CAN_PARTIAL_AGG is not set.  I think that's
wrong.  If can_partial_agg() isn't accurately determining whether
partial aggregation is possible, and as Ashutosh and I have been
discussing, there's room for improvement in that area, then that's a
topic for some other set of patches.  Also, the test in
create_ordinary_grouping_paths for whether or not to call
create_partial_grouping_paths() is super-complicated and uncommented.
I think a simpler approach is to allow create_partial_grouping_paths()
the option of returning NULL.  See 0003, attached.

make_grouping_rel() claims that "For now, all aggregated paths are
added to the (GROUP_AGG, NULL) upperrel", but this is false: we no
longer have only one grouped upper rel.

I'm having a heck of a time understanding what is_partial_aggregation
and perform_partial_partitionwise_aggregation are supposed to be
doing.  It seems like is_partial_aggregation means that we should ONLY
do partial aggregation, which is not exactly what the name implies.
It also seems like perform_partial_partitionwise_aggregation and
is_partial_aggregation differ only in that they apply to the current
level and to the child level respectively; can't we merge these
somehow so that we don't need both of them?

I think that if the last test in can_partitionwise_grouping were moved
before the previous test, it could be simplified to test only
(extra->flags & GROUPING_CAN_PARTIAL_AGG) == 0 and not
*perform_partial_partitionwise_aggregation.

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


0003-Allow-create_partial_grouping_paths-to-return-NULL.patch
Description: Binary data


0002-Fix-Assert-style.patch
Description: Binary data


0001-Remove-loop-from-0-to-1.patch
Description: Binary data

Re: [PROPOSAL] Shared Ispell dictionaries

[Tomas Vondra]

On 03/20/2018 02:11 PM, Arthur Zakirov wrote:
> Hello,
> 
> On Mon, Mar 19, 2018 at 08:50:46PM +0100, Tomas Vondra wrote:
>> Hi Arthur,
>>
>> I went through the patch - just skimming through the diffs, will do more
>> testing tomorrow. Here are a few initial comments.
> 
> Thank you for the review!
> 
>> 1) max_shared_dictionaries_size / PGC_POSTMASTER
>>
>> I'm not quite sure why the GUC is defined as PGC_POSTMASTER, i.e. why it
>> can't be changed after server start. That seems like a fairly useful
>> thing to do (e.g. increase the limit while the server is running), and
>> after looking at the code I think it shouldn't be difficult to change.
> 
> max_shared_dictionaries_size is defined as PGC_SIGHUP now. Added check
> of a new value to disallow to set zero if there are loaded dictionaries
> and to decrease maximum allowed size if loaded size is greater than the
> new value.
> 

I wonder if these restrictions needed? I mean, why not to allow setting
max_shared_dictionaries_size below the size of loaded dictionaries?

Of course, on the one hand those restriction seem sensible. On the other
hand, perhaps in some cases it would be useful to allow violating them?

I mean, why not to simply disable loading of new dictionaries when

(max_shared_dictionaries_size < loaded_size)

Maybe I'm over-thinking this though. It's probably safer and less
surprising to enforce the restrictions.

regards

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

Re: configure's checks for --enable-tap-tests are insufficient

[Alvaro Herrera]

Christoph Berg wrote:
> Re: Alvaro Herrera 2018-03-20 <20180320185038.r3hjvfio4elnaqku@alvherre.pgsql>
> > Time::HiRes is also in libperl5.24, so as far Debian goes, that one
> > would be unnecessary; but IPC::Run and Test::More are in separate
> > packages (libipc-run-perl and libtest-base-perl, respectively.)
> 
> The Debian package build-depends on these:
> 
>  libipc-run-perl,
>  libperl-dev,
>  perl (>= 5.8),
> 
> Test::More is provided by several packages:
> 
> perl-modules-5.24: /usr/share/perl/5.24.1/Test/More.pm
> perl-modules-5.26: /usr/share/perl/5.26.1/Test/More.pm
> libtest-simple-perl: /usr/share/perl5/Test/More.pm
> 
> ... so depending on "perl" is enough, which pulls in
> perl-modules-$current.

Actually, the whole point of this exercise is to ensure that configure
complains properly if somebody installs perl-base but not perl, that is,
what happens if perl-modules-5.24 is not there.

I think you need libperl5.xx in order to use --with-perl; and if you
install that one, you're going to get perl-modules-5.xx as a side
effect, so that scenario is covered.  I don't have a minimal Debian
installation to try
  configure --without-perl --enable-tap-tests

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

Re: [HACKERS] VACUUM and ANALYZE disagreeing on what reltuples means

[Tomas Vondra]

Hi,

So here is an updated version of the patch/fix, addressing the remaining
issues in v3 posted by Tom in November.


The 0001 part is actually a bugfix in bloom and spgist index AM, which
did something like this:

reltuples = IndexBuildHeapScan(...)

result->heap_tuples = result->index_tuples = reltuples;

That is, these two AMs simply used the number of heap rows for the
index. That does not work for partial indexes, of course, where the
correct index reltuples value is likely much lower.

0001 fixes this by tracking the number of actually indexed rows in the
build states, just like in the other index AMs.

A VACUUM or ANALYZE will fix the estimate, of course, but for tables
that are not changing very much it may take quite a while. So I think
this is something we definitely need to back-patch.


The 0002 part is the main part, unifying the definition of reltuples on
three main places:

 a) acquire_sample_rows (ANALYZE)
 b) lazy_scan_heap (VACUUM)
 c) IndexBuildHeapRangeScan (CREATE INDEX)

As the ANALYZE case seems the most constrained, the other two places
were updated to use the same criteria for which rows to include in the
reltuples estimate:

  * HEAPTUPLE_LIVE
  * HEAPTUPLE_INSERT_IN_PROGRESS (same transaction)
  * HEAPTUPLE_DELETE_IN_PROGRESS (not the same trasaction)

This resolves the issue with oscillating reltuples estimates, produced
by VACUUM and ANALYZE (with many non-removable dead tuples).

I've checked all IndexBuildHeapRangeScan callers, and none of them is
using the reltuples estimate for anything except for passing it to
index_update_stats. Aside from the bug fixed in 0001, of course.



regards

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


0001-Compute-index_tuples-correctly-in-bloom-and-spgist.patch.gz
Description: application/gzip


0002-Unify-the-definition-of-reltuples-in-VACUUM-ANALYZE-.patch.gz
Description: application/gzip

Re: configure's checks for --enable-tap-tests are insufficient

[Christoph Berg]

Re: Alvaro Herrera 2018-03-20 <20180320185038.r3hjvfio4elnaqku@alvherre.pgsql>
> Time::HiRes is also in libperl5.24, so as far Debian goes, that one
> would be unnecessary; but IPC::Run and Test::More are in separate
> packages (libipc-run-perl and libtest-base-perl, respectively.)

The Debian package build-depends on these:

 libipc-run-perl,
 libperl-dev,
 perl (>= 5.8),

Test::More is provided by several packages:

perl-modules-5.24: /usr/share/perl/5.24.1/Test/More.pm
perl-modules-5.26: /usr/share/perl/5.26.1/Test/More.pm
libtest-simple-perl: /usr/share/perl5/Test/More.pm

... so depending on "perl" is enough, which pulls in
perl-modules-$current.

> As for the others Dagfinn mentioned, most seem to be in perl-base, and a
> few in perl-modules-5.24, which is depended upon by libperl5.24, so I
> think the proposed set is good enough for now.

Christoph

Re: configure's checks for --enable-tap-tests are insufficient

[Tom Lane]

Andres Freund  writes:
> It's also certainly annoying that AX_PROG_PERL_MODULES doesn't support
> caching.

Yeah, that was the main reason I didn't want to add wholesale module
probing.  Improving that script is no doubt possible, but it's beyond
what I care to do about the issue.

regards, tom lane

Re: configure's checks for --enable-tap-tests are insufficient

[Andres Freund]

On 2018-03-20 17:23:26 +, Dagfinn Ilmari Mannsåker wrote:
> Tom Lane  writes:
> > That seems like expensive overkill to me, especially since it seems
> > unlikely that we'd keep the list up-to-date over time. 
> 
> The patches uptread add about 0.2s to a 6.9s configure run here, I don't
> consider that very expensive.  We don't add new modules very often, so I
> don't think it's much of a maintenance overhead either.

Not huge, but also not nothing. But I think if we care we could make
this cheaper fairly easily? I don't think there's any need to invoke
perl one-by-one for each module. Should be trivial to have perl report
back all the modules it's missing from a list of arguments.

It's also certainly annoying that AX_PROG_PERL_MODULES doesn't support
caching. I'm not sure we're benefiting much from using the AX* script
here.

Greetings,

Andres Freund

Re: configure's checks for --enable-tap-tests are insufficient

[Dagfinn Ilmari Mannsåker]

Alvaro Herrera  writes:

> Tom Lane wrote:
>
>> Ah, thanks.  Not sure we need to make an explicit test for Opcode; we've
>> not heard of anyone not having that.
>
> In Debian stable, Opcode is in package libperl5.24.  This probably
> explains why nobody runs into trouble with it, since libperl is already
> checked by configure for other reasons.  Not sure about the RH world.

Yes, libperl5.xx is pulled in by libperl-dev, which is needed for
PL/Perl anyway.

>> Anyway, I propose the attached.  It produces output like
>> 
>> checking for perl module IPC::Run... ok
>> checking for perl module Test::More 0.87... no
>> checking for perl module Time::HiRes... ok
>> configure: error: Additional Perl modules are required to run TAP tests
>
> This seems good to me.
>
> Time::HiRes is also in libperl5.24, so as far Debian goes, that one
> would be unnecessary;

libperl5.xx is not pulled in if you only have perl-base, you need to
install 'perl' to get that. But Debian isn't really the problem, because
when we say we require perl, people do 'apt install perl' which gives
them the whole shebang.

> but IPC::Run and Test::More are in separate packages (libipc-run-perl
> and libtest-base-perl, respectively.)

Test::More is in perl-modules-5.xx, which is pulled in by 'perl'.

> As for the others Dagfinn mentioned, most seem to be in perl-base, and a
> few in perl-modules-5.24, which is depended upon by libperl5.24, so I
> think the proposed set is good enough for now.

I agree.

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

Re: PQHost() undefined behavior if connecting string contains both host and hostaddr types

[Peter Eisentraut]

On 3/16/18 00:03, Kyotaro HORIGUCHI wrote:
> I agree to the conclusion that PQhost() shouldn't return hostaddr
> "if it has any host name to return". But I still haven't found
> the reason for returning '/tmp' for IP connection.
> 
> The attached patch is revised version of that in the following thread.

That patch looks good to me.

Moreover, I wonder whether we shouldn't remove the branch where
conn->connhost is NULL.  When would that be the case?  The current
behavior is to sometimes return the actual host connected to, and
sometimes the host list.  That doesn't make sense.

I think we should also revert 64f86fb11e20b55fb742af72d55806f8bdd9cd2d,
in which psql's \conninfo digs out the raw hostaddr value to display,
which could contain a list of things.  I think \conninfo should just
display the value returned by PQhost(), and if we change PQhost() to
take hostaddr into account, then this should address the original complaint.

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

Re: configure's checks for --enable-tap-tests are insufficient

[Tom Lane]

Alvaro Herrera  writes:
> Tom Lane wrote:
>> Ah, thanks.  Not sure we need to make an explicit test for Opcode; we've
>> not heard of anyone not having that.

> In Debian stable, Opcode is in package libperl5.24.  This probably
> explains why nobody runs into trouble with it, since libperl is already
> checked by configure for other reasons.  Not sure about the RH world.

[ pokes around ]  Opcode is in the base perl package on RHEL6;
on Fedora 28, it's in perl-interpreter.  Either way, it's in the same
package as /usr/bin/perl, so ain't nobody not gonna have it.

> As for the others Dagfinn mentioned, most seem to be in perl-base, and a
> few in perl-modules-5.24, which is depended upon by libperl5.24, so I
> think the proposed set is good enough for now.

Sounds good.  We can always tweak the list some more if the situation
changes, or if we hear from someone using a different distro that slices
things up yet differently.  But I don't feel a need to expend configure
cycles on purely hypothetical configurations.  I imagine that most
off-the-beaten-track cases would involve hand-built Perl installations,
and those will almost certainly have all of "Perl core".  So I think
as long as we cover what the major Linux distros are doing, that's
probably good enough.

regards, tom lane

Re: [HACKERS] plpgsql - additional extra checks

[Pavel Stehule]

2018-03-20 13:56 GMT+01:00 Tels :

> Hello Pavel and Tomas,
>
> On Tue, March 20, 2018 12:36 am, Pavel Stehule wrote:
> > 2018-03-19 21:47 GMT+01:00 Tomas Vondra :
> >
> >> Hi,
> >>
> >> I'm looking at the updated patch (plpgsql-extra-check-180316.patch),
> and
> >> this time it applies and builds OK. The one thing I noticed is that the
> >> documentation still uses the old wording for strict_multi_assignement:
> >>
> >> WARNING:  Number of evaluated fields does not match expected.
> >> HINT:  strict_multi_assignement check of extra_warnings is active.
> >> WARNING:  Number of evaluated fields does not match expected.
> >> HINT:  strict_multi_assignement check of extra_warnings is active.
> >>
> >> This was reworded to "Number of source and target fields in assignment
> >> does not match."
> >>
>
> I believe the correct wording should be:
>
> "Number of source and target fields in assignment do not match."
>
> ecause comparing one number to the other means "the number A and B _do_
> not match", not "the number A does not match number B".
>
> Also there is an inconsistent quoting here:
>
> +   
> +Setting plpgsql.extra_warnings, or
> +plpgsql.extra_errors, as appropriate, to
> all
>
> no quotes for 'all'.
>
> +is encouraged in development and/or testing environments.
> +   
> +
> +   
> +These additional checks are enabled through the configuration
> variables
> +plpgsql.extra_warnings for warnings and
> +plpgsql.extra_errors for errors. Both can be set
> either to
> +a comma-separated list of checks, "none" or
> +"all".
>
> quotes here around '"all"'. I think it should be one or the other in both
> cases.
>
> Also:
>
> + Currently
> +the list of available checks includes only one:
>
> but then it lists more than one check?
>

all mentioned issues should be fixed

Thank you for your time :)

Regards

Pavel


>
> Best wishes,
>
> Tels
>
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 7ed926fd51..807e4a66a1 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -5003,7 +5003,7 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$
 
   
   
-   Additional Compile-time Checks
+   Additional Compile-time and Run-time Checks
 

 To aid the user in finding instances of simple but common problems before
@@ -5015,26 +5015,62 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$
 so you are advised to test in a separate development environment.

 
- 
-  These additional checks are enabled through the configuration variables
-  plpgsql.extra_warnings for warnings and
-  plpgsql.extra_errors for errors. Both can be set either to
-  a comma-separated list of checks, "none" or "all".
-  The default is "none". Currently the list of available checks
-  includes only one:
-  
-   
-shadowed_variables
-
- 
-  Checks if a declaration shadows a previously defined variable.
- 
-
-   
-  
+   
+Setting plpgsql.extra_warnings, or
+plpgsql.extra_errors, as appropriate, to "all"
+is encouraged in development and/or testing environments.
+   
+
+   
+These additional checks are enabled through the configuration variables
+plpgsql.extra_warnings for warnings and
+plpgsql.extra_errors for errors. Both can be set either to
+a comma-separated list of checks, "none" or
+"all". The default is "none". Currently
+the list of available checks includes:
+
+ 
+  shadowed_variables
+  
+   
+Checks if a declaration shadows a previously defined variable.
+   
+  
+ 
 
-  The following example shows the effect of plpgsql.extra_warnings
-  set to shadowed_variables:
+ 
+  strict_multi_assignment
+  
+   
+Some PL/PgSQL commands allow assigning values
+to more than one variable at a time, such as SELECT INTO.  Typically,
+the number of target variables and the number of source variables should
+match, though PL/PgSQL will use NULL for
+missing values and extra variables are ignored.  Enabling this check
+will cause PL/PgSQL to throw a WARNING or
+ERROR whenever the number of target variables and the number of source
+variables are different.
+   
+  
+ 
+
+ 
+  too_many_rows
+  
+   
+Enabling this check will cause PL/PgSQL to
+check if a given query returns more than one row when an
+INTO clause is used.  As an INTO statement will only
+ever use one row, having a query return multiple rows is generally
+either inefficient and/or nondeterministic and therefore is likely an
+error.
+   
+  
+ 
+
+
+The following example shows the effect of plpgsql.extra_warnings
+set to shadowed_variables:
 
 SET plpgsql.extra_warnings TO 'shadowed_variables';
 
@@ -5050,8 +5086,38 @@ LINE 3: f1 

Re: pgsql: Fix CommandCounterIncrement in partition-related DDL

[Alvaro Herrera]

Tom Lane wrote:
> Alvaro Herrera  writes:
> > I wonder about adding a syscache callback so that when an item in
> > pg_partitioned_table is invalidated, the relcache entry for partrelid
> > entry in pg_class is invalidated also.  I can't find any precedent for
> > anything similar, though, and there doesn't seem to be any convenient
> > place to do it, either.
> 
> In principle you could do it by adding logic to CacheInvalidateHeapTuple
> in inval.c, similar to the existing logic for pg_class, pg_attribute
> and pg_index entries.  Not sure it's worthwhile though.  That's very
> ancient code; of late our practice has been to insist that the code
> modifying other catalogs that feed into relcache entries should issue
> a relcache inval explicitly.  If there's a reason why it's not convenient
> to do that, then maybe making CacheInvalidateHeapTuple do it is a
> good way.

Actually, the current code uses CacheInvalidateRelcache() already and it
seems to work; I was looking for a better way that did not require us to
remember it for every update in that catalog, but it seems there isn't
one.

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

Re: configure's checks for --enable-tap-tests are insufficient

[Alvaro Herrera]

Tom Lane wrote:
> ilm...@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
> > Tom Lane  writes:
> >> I just want to test for modules that we know are likely to be omitted
> >> on popular platforms.  I've proposed testing two that would improve
> >> the user experience on Red Hat; what's the equivalent minimum set for
> >> Debian?
> 
> > On Debian (I've checked the previous and current stable releases, as
> > well as unstable) the only package required by the TAP tests not in
> > perl-base is Time::HiRes.  Of the modules required by PL/Perl, Opcode is
> > not in perl-base either.
> 
> Ah, thanks.  Not sure we need to make an explicit test for Opcode; we've
> not heard of anyone not having that.

In Debian stable, Opcode is in package libperl5.24.  This probably
explains why nobody runs into trouble with it, since libperl is already
checked by configure for other reasons.  Not sure about the RH world.

> Anyway, I propose the attached.  It produces output like
> 
> checking for perl module IPC::Run... ok
> checking for perl module Test::More 0.87... no
> checking for perl module Time::HiRes... ok
> configure: error: Additional Perl modules are required to run TAP tests

This seems good to me.

Time::HiRes is also in libperl5.24, so as far Debian goes, that one
would be unnecessary; but IPC::Run and Test::More are in separate
packages (libipc-run-perl and libtest-base-perl, respectively.)

As for the others Dagfinn mentioned, most seem to be in perl-base, and a
few in perl-modules-5.24, which is depended upon by libperl5.24, so I
think the proposed set is good enough for now.

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

Re: constraint exclusion and nulls in IN (..) clause

[Tom Lane]

I wrote:
> After further thought, it seems like the place to deal with this is
> really operator_predicate_proof(), as in the attached draft patch
> against HEAD.  This passes the smell test for me, in the sense that
> it's an arguably correct and general extension of the proof rules,
> but it could use more testing.

Was anyone planning to do more work or testing on this?  Or should
I just push it so we can close the CF entry?

regards, tom lane

Re: [HACKERS] per-sesson errors after interrupting CLUSTER pg_attribute (not attrdef)

[Justin Pryzby]

On Tue, Mar 20, 2018 at 01:03:57PM -0500, Justin Pryzby wrote:
> On Tue, Oct 24, 2017 at 04:56:27PM -0700, Michael Paquier wrote:
> > On Fri, Oct 20, 2017 at 9:01 AM, Justin Pryzby  wrote:
> > > This was briefly scary but seems to have been limited to my psql session 
> > > (no
> > > other errors logged).  Issue with catcache (?)
> 
> Now I know, this is still an issue under PG10.2:
> 
> [pryzbyj@united-telsasoft ~]$ psql ts
> psql (10.2)
> ts=# CLUSTER VERBOSE pg_attribute USING pg_attribute_relid_attnum_index ;
> INFO:  clustering "pg_catalog.pg_attribute" using sequential scan and sort
> INFO:  "pg_attribute": found 18102 removable, 2488511 nonremovable row 
> versions in 80769 pages
> DETAIL:  0 dead row versions cannot be removed yet.
> CPU: user: 10.00 s, system: 1.59 s, elapsed: 27.60 s.
> ^CCancel request sent
> ERROR:  canceling statement due to user request
> ts=# CLUSTER VERBOSE pg_attribute USING pg_attribute_relid_attnum_index ;
> ERROR:  could not open file "base/16400/948150297": No such file or directory

> I don't know much but..is this related to pg_filenode.map?

I gather this is the "prefer that it not happen" case here:
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/utils/cache/relmapper.c;hb=HEAD#l449

So I think this is a known, accepted/expected behavior and no issue.

Justin

Re: pgsql: Fix CommandCounterIncrement in partition-related DDL

[Tom Lane]

Alvaro Herrera  writes:
> I wonder about adding a syscache callback so that when an item in
> pg_partitioned_table is invalidated, the relcache entry for partrelid
> entry in pg_class is invalidated also.  I can't find any precedent for
> anything similar, though, and there doesn't seem to be any convenient
> place to do it, either.

In principle you could do it by adding logic to CacheInvalidateHeapTuple
in inval.c, similar to the existing logic for pg_class, pg_attribute
and pg_index entries.  Not sure it's worthwhile though.  That's very
ancient code; of late our practice has been to insist that the code
modifying other catalogs that feed into relcache entries should issue
a relcache inval explicitly.  If there's a reason why it's not convenient
to do that, then maybe making CacheInvalidateHeapTuple do it is a
good way.

regards, tom lane

Re: pgsql: Fix CommandCounterIncrement in partition-related DDL

[Alvaro Herrera]

Alvaro Herrera wrote:
> Alvaro Herrera wrote:
> > Fix CommandCounterIncrement in partition-related DDL
> 
> Hmm, Prion seems unhappy about this.  Looking

Here's a patch that seems to fix the problem, and generally looks sane
to me.

The previous arrangements to avoid CCI seem brittle: apparently we were
forced to do StorePartitionBounds() and immediately
update_default_partition_oid() without CCI in between, or various things
went nuts ("unexpected partdefid").  With this patch, what we do is we
call update_default_partition_oid *within* StorePartitionBounds without
an intervening CCI, which seems to achieve the same result -- namely
that when the relcache entry is rebuilt, it doesn't end up incomplete.

The two places that previously called update_default_partition_oid with
a non-zero default partition OID no longer call it, since they were
storing bounds.  The only places that remain outside of
StorePartitionBounds call it with InvalidOid (during relation drop, and
during partition detach).

I also remove a crock in RelationBuildPartitionDesc to return empty when
"key is null" (i.e. pg_class entry was there but not
pg_partitioned_table), which makes no sense -- except if you're building
the cache untimely, which apparently is what was happening.  If you make
sure the pg_partitioned_table entry is present before making the
pg_class entry visible, then this should not be necessary.

heap_drop_with_catalog contains a copy-paste failure, fixed also in this
patch.

I wonder about adding a syscache callback so that when an item in
pg_partitioned_table is invalidated, the relcache entry for partrelid
entry in pg_class is invalidated also.  I can't find any precedent for
anything similar, though, and there doesn't seem to be any convenient
place to do it, either.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 0497332e9d..a049234390 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -1769,7 +1769,8 @@ heap_drop_with_catalog(Oid relid)
 * could attempt to access the just-dropped relation as its partition. 
We
 * must therefore take a table lock strong enough to prevent all queries
 * on the table from proceeding until we commit and send out a
-* shared-cache-inval notice that will make them update their index 
lists.
+* shared-cache-inval notice that will make them update their partition
+* descriptors.
 */
tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
if (!HeapTupleIsValid(tuple))
@@ -3247,6 +3248,9 @@ RemovePartitionKeyByRelId(Oid relid)
  * Update pg_class tuple of rel to store the partition bound and 
set
  * relispartition to true
  *
+ * If this is the default partition, also update the default partition OID in
+ * pg_partitioned_table.
+ *
  * Also, invalidate the parent's relcache, so that the next rebuild will load
  * the new partition's info into its partition descriptor.  If there is a
  * default partition, we must invalidate its relcache entry as well.
@@ -3298,7 +3302,17 @@ StorePartitionBound(Relation rel, Relation parent, 
PartitionBoundSpec *bound)
heap_freetuple(newtuple);
heap_close(classRel, RowExclusiveLock);
 
-   /* Make update visible */
+   /*
+* If we're storing bounds for the default partition, update
+* pg_partitioned_table too.  This must happen before CCI, to avoid risk
+* of constructing a partition descriptor lacking the default partition
+* OID.
+*/
+   if (bound->is_default)
+   update_default_partition_oid(RelationGetRelid(parent),
+
RelationGetRelid(rel));
+
+   /* Make these updates visible */
CommandCounterIncrement();
 
/*
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 786c05df73..53855f5088 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -227,13 +227,6 @@ RelationBuildPartitionDesc(Relation rel)
/* Range partitioning specific */
PartitionRangeBound **rbounds = NULL;
 
-   /*
-* The following could happen in situations where rel has a pg_class 
entry
-* but not the pg_partitioned_table entry yet.
-*/
-   if (key == NULL)
-   return;
-
/* Get partition oids from pg_inherits */
inhoids = find_inheritance_children(RelationGetRelid(rel), NoLock);
 
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8f83aa4675..c4cf0abadc 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -859,10 +859,6 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
/* Update the pg_class entry. */
   

Re: configure's checks for --enable-tap-tests are insufficient

[Tom Lane]

ilm...@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
> Tom Lane  writes:
>> I just want to test for modules that we know are likely to be omitted
>> on popular platforms.  I've proposed testing two that would improve
>> the user experience on Red Hat; what's the equivalent minimum set for
>> Debian?

> On Debian (I've checked the previous and current stable releases, as
> well as unstable) the only package required by the TAP tests not in
> perl-base is Time::HiRes.  Of the modules required by PL/Perl, Opcode is
> not in perl-base either.

Ah, thanks.  Not sure we need to make an explicit test for Opcode; we've
not heard of anyone not having that.

> BTW, should include the version in the Test::More check, since we only
> requie Perl 5.8, but that only shipped Test::More 0.47, while we require
> 0.97.

Good point, though it looks to me like we're requiring 0.87 not 0.97?

Anyway, I propose the attached.  It produces output like

checking for perl module IPC::Run... ok
checking for perl module Test::More 0.87... no
checking for perl module Time::HiRes... ok
configure: error: Additional Perl modules are required to run TAP tests

which seems a better approach to me than making the user find out one
at a time which modules are needed.

regards, tom lane

diff --git a/configure b/configure
index 3943711..1be8a1a 100755
*** a/configure
--- b/configure
*** if test "$enable_tap_tests" = yes; then
*** 16517,16522 
--- 16517,16528 
# (prove might be part of a different Perl installation than perl, eg on
# MSys, so the result of AX_PROG_PERL_MODULES could be irrelevant anyway.)
if test -z "$PROVE"; then
+ # Test::More and Time::HiRes are supposed to be part of core Perl,
+ # but some distros omit them in a minimal installation.
+ 
+ 
+ 
+ 
  
  
  
*** fi
*** 16566,16572 
  
  if test "x$PERL" != x; then
ax_perl_modules_failed=0
!   for ax_perl_module in 'IPC::Run' ; do
  { $as_echo "$as_me:${as_lineno-$LINENO}: checking for perl module $ax_perl_module" >&5
  $as_echo_n "checking for perl module $ax_perl_module... " >&6; }
  
--- 16572,16578 
  
  if test "x$PERL" != x; then
ax_perl_modules_failed=0
!   for ax_perl_module in 'IPC::Run' 'Test::More 0.87' 'Time::HiRes' ; do
  { $as_echo "$as_me:${as_lineno-$LINENO}: checking for perl module $ax_perl_module" >&5
  $as_echo_n "checking for perl module $ax_perl_module... " >&6; }
  
*** $as_echo "ok" >&6; };
*** 16588,16594 
  
else
  :
! as_fn_error $? "Perl module IPC::Run is required to run TAP tests" "$LINENO" 5
fi
  else
{ $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: could not find perl" >&5
--- 16594,16600 
  
else
  :
! as_fn_error $? "Additional Perl modules are required to run TAP tests" "$LINENO" 5
fi
  else
{ $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: could not find perl" >&5
diff --git a/configure.in b/configure.in
index 1babdbb..9a6f4b1 100644
*** a/configure.in
--- b/configure.in
*** if test "$enable_tap_tests" = yes; then
*** 2114,2121 
# (prove might be part of a different Perl installation than perl, eg on
# MSys, so the result of AX_PROG_PERL_MODULES could be irrelevant anyway.)
if test -z "$PROVE"; then
! AX_PROG_PERL_MODULES(IPC::Run, ,
!   AC_MSG_ERROR([Perl module IPC::Run is required to run TAP tests]))
fi
# Now make sure we know where prove is
PGAC_PATH_PROGS(PROVE, prove)
--- 2114,2123 
# (prove might be part of a different Perl installation than perl, eg on
# MSys, so the result of AX_PROG_PERL_MODULES could be irrelevant anyway.)
if test -z "$PROVE"; then
! # Test::More and Time::HiRes are supposed to be part of core Perl,
! # but some distros omit them in a minimal installation.
! AX_PROG_PERL_MODULES([IPC::Run Test::More=0.87 Time::HiRes], ,
!   [AC_MSG_ERROR([Additional Perl modules are required to run TAP tests])])
fi
# Now make sure we know where prove is
PGAC_PATH_PROGS(PROVE, prove)

Re: [HACKERS] per-sesson errors after interrupting CLUSTER pg_attribute (not attrdef)

[Justin Pryzby]

On Tue, Oct 24, 2017 at 04:56:27PM -0700, Michael Paquier wrote:
> On Fri, Oct 20, 2017 at 9:01 AM, Justin Pryzby  wrote:
> > This was briefly scary but seems to have been limited to my psql session (no
> > other errors logged).  Issue with catcache (?)
> >
> > I realized that the backup job I'd kicked off was precluding the CLUSTER 
> > from
> > running, but that CLUSTER was still holding lock and stalling everything 
> > else
> > under the sun.
> >
> > ts=# \df qci_add*
> > ERROR:  could not read block 8 in file "base/16400/999225102": read only 0 
> > of 8192 bytes
> > ts=# \dt+ pg_att
> >
> > ts=# \dt+ pg_attrdef
> > ERROR:  could not read block 8 in file "base/16400/999225102": read only 0 
> > of 8192 bytes
> 
> Perhaps that's too late, but to which relation is this relfilenode
> actually referring to?

Sorry for the late response :)

..but I've had mixed success reproducign this, and wasn't sure if it even an
issue under current postgres (the original issue was a 9.6 server+10.0client).

Now I know, this is still an issue under PG10.2:

[pryzbyj@united-telsasoft ~]$ psql ts
psql (10.2)
Type "help" for help.

ts=# CLUSTER VERBOSE pg_attribute USING pg_attribute_relid_attnum_index ;
INFO:  clustering "pg_catalog.pg_attribute" using sequential scan and sort
INFO:  "pg_attribute": found 18074 removable, 2488511 nonremovable row versions 
in 80769 pages
DETAIL:  0 dead row versions cannot be removed yet.
CPU: user: 9.66 s, system: 1.92 s, elapsed: 22.29 s.
^CCancel request sent
ERROR:  canceling statement due to user request
ts=# CLUSTER VERBOSE pg_attribute USING pg_attribute_relid_attnum_index ;
INFO:  clustering "pg_catalog.pg_attribute" using sequential scan and sort
INFO:  "pg_attribute": found 18102 removable, 2488511 nonremovable row versions 
in 80769 pages
DETAIL:  0 dead row versions cannot be removed yet.
CPU: user: 10.00 s, system: 1.59 s, elapsed: 27.60 s.
^CCancel request sent
ERROR:  canceling statement due to user request
ts=# CLUSTER VERBOSE pg_attribute USING pg_attribute_relid_attnum_index ;
ERROR:  could not open file "base/16400/948150297": No such file or directory
ts=# SELECT * FROM pg_attribute WHERE pg_relation_filenode=948150297;
ERROR:  column "pg_relation_filenode" does not exist
LINE 1: SELECT * FROM pg_attribute WHERE pg_relation_filenode=948150...
 ^
ts=# SELECT * FROM pg_attribute WHERE pg_relation_filenode()=948150297;
ERROR:  function pg_relation_filenode() does not exist
LINE 1: SELECT * FROM pg_attribute WHERE pg_relation_filenode()=9481...
 ^
HINT:  No function matches the given name and argument types. You might need to 
add explicit type casts.
ts=# SELECT * FROM pg_class WHERE pg_relation_filenode(oid)=948150297;
ERROR:  could not open file "base/16400/948150297": No such file or directory
ts=# 

In another session:
ts=# SELECT * FROM pg_class WHERE pg_relation_filenode(oid)=948150297;
 relname | relnamespace | reltype | reloftype | relowner | relam | relfilenode 
| reltablespace | relpages | reltuples | relallvisible | reltoastrelid | 
relhasindex | relisshared | relpersistence | relkind | relnatts | relchecks | 
relhasoids | rel
haspkey | relhasrules | relhastriggers | relhassubclass | relrowsecurity | 
relforcerowsecurity | relispopulated | relreplident | relispartition | 
relfrozenxid | relminmxid | relacl | reloptions | relpartbound
-+--+-+---+--+---+-+---+--+---+---+---+-+-++-+--+---++
+-++++-++--++--++++--
(0 rows)

postgres=# SELECT session_line, message, query FROM 
postgres_log_2018_03_20_1200 WHERE pid=29855 AND session_id='5ab147d9.749f' 
ORDER BY 1;
session_line|message|query
1|statement: SELECT pg_catalog.quote_ident(c2.relname)   FROM 
pg_catalog.pg_class c1, pg_catalog.pg_class c2, pg_catalog.pg_index i WHERE 
c1.oid=i.indrelid and i.indexrelid=c2.oid   and (0 = pg_catalog.length('')) 
  and pg_catalog.quote_ident(c1.relname)='pg_attribute'   and 
pg_catalog.pg_table_is_visible(c2.oid)
LIMIT 1000|
2|statement: SELECT pg_catalog.quote_ident(c2.relname)   FROM 
pg_catalog.pg_class c1, pg_catalog.pg_class c2, pg_catalog.pg_index i WHERE 
c1.oid=i.indrelid and i.indexrelid=c2.oid   and (24 = 
pg_catalog.length('pg_attribute_relid_attnu'))   and 
pg_catalog.quote_ident(c1.relname)='pg_attribute'   and 
pg_catalog.pg_table_is_visible(c2.oid)
LIMIT 1000|
3|statement: CLUSTER VERBOSE pg_attribute USING pg_attribute_relid_attnum_index 
;|
4|clustering "pg_catalog.pg_attribute" using sequential scan and sort|
5|temporary file: path 

Re: [PATCH] session_replication_role = replica with TRUNCATE

[Peter Eisentraut]

So, what should we do here?  The argument of the patch is that it makes
the behavior of TRUNCATE consistent with DELETE, which would be good.
But my argument is that the behavior of DELETE is kind of bad, and we
should create more stuff like that.  I also haven't seen an argument why
this change is actually necessary.

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

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Tom Lane]

So here's what I think we should do about this:

1. Only GUC_LIST_QUOTE variables need to be special-cased.  It works
fine to treat plain LIST variables (e.g. datestyle) with the regular
code path.  This is good because there are so few of them.

2. We should make an effort to minimize the number of places that know
explicitly which variables are GUC_LIST_QUOTE.  In the backend, the
right thing to do is ask guc.c.  In pg_dump, there's no convenient
way to ask the backend (and certainly nothing that would work against
old servers), but we should at least make sure there's only one place
to fix not two.

3. We need to prevent extensions from trying to create GUC_LIST_QUOTE
variables, because those are not going to work correctly if they're
set before the extension is loaded, nor is there any hope of pg_dump
dumping them correctly.

The attached 0001 patch does the first two things, and could be
back-patched.  The 0002 patch, which I'd only propose for HEAD,
is one way we could address point 3.  A different way would be
to throw a WARNING rather than ERROR and then just mask off the
problematic bit.  Or we could decide that either of these cures
is worse than the disease, but I don't really agree with that.

Comments?

regards, tom lane

diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 2cd54ec..b0559ca 100644
*** a/src/backend/utils/adt/ruleutils.c
--- b/src/backend/utils/adt/ruleutils.c
***
*** 61,66 
--- 61,67 
  #include "utils/array.h"
  #include "utils/builtins.h"
  #include "utils/fmgroids.h"
+ #include "utils/guc.h"
  #include "utils/hsearch.h"
  #include "utils/lsyscache.h"
  #include "utils/rel.h"
*** pg_get_functiondef(PG_FUNCTION_ARGS)
*** 2597,2607 
   quote_identifier(configitem));
  
  /*
!  * Some GUC variable names are 'LIST' type and hence must not
!  * be quoted.
   */
! if (pg_strcasecmp(configitem, "DateStyle") == 0
! 	|| pg_strcasecmp(configitem, "search_path") == 0)
  	appendStringInfoString(, pos);
  else
  	simple_quote_literal(, pos);
--- 2598,2612 
   quote_identifier(configitem));
  
  /*
!  * Variables that are marked GUC_LIST_QUOTE were already fully
!  * quoted by flatten_set_variable_args() before they were put
!  * into the proconfig array; we mustn't re-quote them or we'll
!  * make a mess.  Variables that are not so marked should just
!  * be emitted as simple string literals.  If the variable is
!  * not known to guc.c, we'll do the latter; this makes it
!  * unsafe to use GUC_LIST_QUOTE for extension variables.
   */
! if (GetConfigOptionFlags(configitem, true) & GUC_LIST_QUOTE)
  	appendStringInfoString(, pos);
  else
  	simple_quote_literal(, pos);
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 7a7ac47..398680a 100644
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*** static const unit_conversion time_unit_c
*** 796,803 
   *
   * 6. Don't forget to document the option (at least in config.sgml).
   *
!  * 7. If it's a new GUC_LIST option you must edit pg_dumpall.c to ensure
!  *	  it is not single quoted at dump time.
   */
  
  
--- 796,803 
   *
   * 6. Don't forget to document the option (at least in config.sgml).
   *
!  * 7. If it's a new GUC_LIST_QUOTE option, you must add it to
!  *	  variable_is_guc_list_quote() in src/bin/pg_dump/dumputils.c.
   */
  
  
*** GetConfigOptionResetString(const char *n
*** 6859,6864 
--- 6859,6888 
  	return NULL;
  }
  
+ /*
+  * Get the GUC flags associated with the given option.
+  *
+  * If the option doesn't exist, return 0 if missing_ok is true,
+  * otherwise throw an ereport and don't return.
+  */
+ int
+ GetConfigOptionFlags(const char *name, bool missing_ok)
+ {
+ 	struct config_generic *record;
+ 
+ 	record = find_option(name, false, WARNING);
+ 	if (record == NULL)
+ 	{
+ 		if (missing_ok)
+ 			return 0;
+ 		ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+  errmsg("unrecognized configuration parameter \"%s\"",
+ 		name)));
+ 	}
+ 	return record->flags;
+ }
+ 
  
  /*
   * flatten_set_variable_args
diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c
index 7f5bb13..875545f 100644
*** a/src/bin/pg_dump/dumputils.c
--- b/src/bin/pg_dump/dumputils.c
*** buildACLQueries(PQExpBuffer acl_subquery
*** 851,856 
--- 851,879 
  }
  
  /*
+  * Detect whether the given GUC variable is of GUC_LIST_QUOTE type.
+  *
+  * It'd be better if we could inquire this directly from the backend; but even
+  * if there were a function for that, it could only tell us about variables
+  * currently known to guc.c, so that it'd be unsafe for extensions to declare
+  * GUC_LIST_QUOTE variables anyway.  Lacking a solution for that, it doesn't
+  * seem worth the work to do more than have this list, 

Re: configure's checks for --enable-tap-tests are insufficient

[Tom Lane]

ilm...@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
> Tom Lane  writes:
>> ilm...@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
>>> I think the other way would be better simpler: we require a complete
>>> Perl core installation.

>> And you propose to test for that in configure how?

> Good point, I'd overlooked that aspect.  In that case we can't rely on
> what Debian or others happen to include in their minimal packages, we
> have to explicitly check for _every_ module we use anywhere in the test
> suite, core or not (except perhaps pragmas like 'strict' and 'warnings').

That seems like expensive overkill to me, especially since it seems
unlikely that we'd keep the list up-to-date over time.  I just want to
test for modules that we know are likely to be omitted on popular
platforms.  I've proposed testing two that would improve the user
experience on Red Hat; what's the equivalent minimum set for Debian?

I think the documentation can reasonably say that we expect perl-core
(plus IPC::Run, if that's not core).  But configure's task is just to
make a reasonable attempt to notice if you're missing something.  If
we have to tweak it again in the future, that's fine with me.

regards, tom lane

Re: configure's checks for --enable-tap-tests are insufficient

[Dagfinn Ilmari Mannsåker]

Tom Lane  writes:

> ilm...@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
>> Tom Lane  writes:
>>> Ah-hah.  So it seems to me that it'd be a useful exercise to start
>>> from perl-base and find out what has to be added to get to working
>>> TAP tests.  Anyone?
>
>> I think the other way would be better simpler: we require a complete
>> Perl core installation.
>
> And you propose to test for that in configure how?

Good point, I'd overlooked that aspect.  In that case we can't rely on
what Debian or others happen to include in their minimal packages, we
have to explicitly check for _every_ module we use anywhere in the test
suite, core or not (except perhaps pragmas like 'strict' and 'warnings').

Grepping the tests (**/t/ src/test/perl/) for 'use' and 'require'
statements and excluding 'strict', 'warnings' and ones defined locally
gives the following list:

Carp
Config
Cwd
Exporter
File::Basename
File::Copy
File::Find
File::Path
File::Spec
File::Temp
IPC::Run
Scalar::Util
Socket
Test::More 0.87
Time::HiRes

I suggest checking for all of those under --enable-tap-tests.

Additionally, plperl uses some modules which we should check for under
--with-perl:

Carp
Getopt::Long
Opcode

Please find attaced separate patches for both of the above.

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

>From 42abdb8cfadda4b50216a9e5130c4ae56a075de5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Tue, 20 Mar 2018 16:27:21 +
Subject: [PATCH 1/2] Check for all modules used by the TAP tests

Several distributions, notably Fedora-based ones, provide a 'perl'
package which doesn't actually ship all Perl core modules.  Guard
against this by checking for all modules we use, including core
ones (but not pragmata like 'strict' and 'warnings') in configure.
---
 configure| 4 ++--
 configure.in | 6 --
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/configure b/configure
index 3943711283..ba736aa262 100755
--- a/configure
+++ b/configure
@@ -16566,7 +16566,7 @@ fi
 
 if test "x$PERL" != x; then
   ax_perl_modules_failed=0
-  for ax_perl_module in 'IPC::Run' ; do
+  for ax_perl_module in 'Carp' 'Config' 'Cwd' 'Exporter' 'File::Basename' 'File::Copy' 'File::Find' 'File::Path' 'File::Spec' 'File::Temp' 'IPC::Run' 'Scalar::Util' 'Socket' 'Test::More 0.87' 'Time::HiRes' ; do
 { $as_echo "$as_me:${as_lineno-$LINENO}: checking for perl module $ax_perl_module" >&5
 $as_echo_n "checking for perl module $ax_perl_module... " >&6; }
 
@@ -16588,7 +16588,7 @@ $as_echo "ok" >&6; };
 
   else
 :
-as_fn_error $? "Perl module IPC::Run is required to run TAP tests" "$LINENO" 5
+as_fn_error $? "Perl modules required to run TAP tests not found" "$LINENO" 5
   fi
 else
   { $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: could not find perl" >&5
diff --git a/configure.in b/configure.in
index 1babdbb755..1152b7f536 100644
--- a/configure.in
+++ b/configure.in
@@ -2114,8 +2114,10 @@ if test "$enable_tap_tests" = yes; then
   # (prove might be part of a different Perl installation than perl, eg on
   # MSys, so the result of AX_PROG_PERL_MODULES could be irrelevant anyway.)
   if test -z "$PROVE"; then
-AX_PROG_PERL_MODULES(IPC::Run, ,
-  AC_MSG_ERROR([Perl module IPC::Run is required to run TAP tests]))
+AX_PROG_PERL_MODULES(Carp Config Cwd Exporter File::Basename File::Copy
+ File::Find File::Path File::Spec File::Temp IPC::Run
+ Scalar::Util Socket Test::More=0.87 Time::HiRes, ,
+  AC_MSG_ERROR([Perl modules required to run TAP tests not found]))
   fi
   # Now make sure we know where prove is
   PGAC_PATH_PROGS(PROVE, prove)
-- 
2.16.2

>From eb6550a6c5813c4334bab86dbe3d838dc3b18b5e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Tue, 20 Mar 2018 16:40:53 +
Subject: [PATCH 2/2] Check for all modules needed by PL/Perl

Several distributions, notably Fedora-based ones, provide a 'perl'
package which doesn't actually ship all Perl core modules.  Guard
against this by checking for all modules we use, including core
ones (but not pragmata like 'strict' and 'warnings') in configure.
---
 configure| 71 
 configure.in |  2 ++
 2 files changed, 73 insertions(+)

diff --git a/configure b/configure
index ba736aa262..eddbdb8849 100755
--- a/configure
+++ b/configure
@@ -7849,6 +7849,77 @@ else
 $as_echo "$perl_embed_ldflags" >&6; }
 fi
 
+# Make sure we have perl
+if test -z "$PERL"; then
+# Extract the first word of "perl", so it can be a program name with args.
+set dummy perl; ac_word=$2
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for $ac_word" 

Re: missing support of named convention for procedures

[Peter Eisentraut]

On 3/16/18 06:29, Pavel Stehule wrote:
> attached patch fixes it

The fix doesn't seem to work for LANGUAGE SQL procedures.  For example:

CREATE PROCEDURE ptest5(a int, b int DEFAULT 0)
LANGUAGE SQL
AS $$
INSERT INTO cp_test VALUES (a, 'foo');
INSERT INTO cp_test VALUES (b, 'bar');
$$;
CALL ptest5(a => 1, b => 2);  -- ok
CALL ptest5(b => 3, a => 4);  -- ok
CALL ptest5(5);
ERROR:  no value found for parameter 2
CONTEXT:  SQL function "ptest5" statement 2
CALL ptest5(a => 6);
ERROR:  no value found for parameter 2
CONTEXT:  SQL function "ptest5" statement 2

I'm not quite sure why this behaves differently from plpgsql.  Needs
more digging.

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

Re: [WIP PATCH] Index scan offset optimisation using visibility map

[Michail Nikolaev]

Hello everyone.

I need an advice.

I was reworking the patch: added support for the planner, added support for
queries with projection, addded support for predicates which could be
executed over index data.
And.. I realized that my IndexScan is even more index-only than the
original IndexOnlyScan. So, it seems to be a wrong way.

I think the task could be splitted into two:

1. Extended support for index-only-scan

Currently IndexOnlyScan is used only in case when target data is fully
located in
index. If we need some additional columns - regular index scan is used
anyway.

For example, let's consider such table and index:

CREATE TABLE test_events (
  time timestamp ,
  event varchar(255),
  data jsonb
);

CREATE INDEX on test_events USING btree(time, event);

It is some kind of big table with log events. And let's consinder such
query:

SELECT data->>'event_id'
FROM test_events
WHERE
time > (now() - interval '2 year') AND -- indexqual
event = 'tax' AND -- indexqual
extract(ISODOW from time) = 1 --qpquals
ORDER BY time DESC

At the moment IndexScan plan will be used for such query due to result
data. But 1/7 of all heap access will be lost. At the same time
"extract(ISODOW from time) = 1" (qpqualsl) could be easily calculated over
index data.

The idea is simple: extend IndexOnly scan to be used if all query
predicates (both indexqual and qpquals) could be calculated over index
data. And if all checks are passed - just load tuple from heap to return.
It seems like index-data access is really cheap now and such plan will
be faster even for qpquals without high selectivity. At least for
READCOMMITED.

I think I will able to create prototype within a few days (most of work
is done in current patch rework).

Probably it is not an ne idea - so, is it worth implementation? Maybe
I've missed something huge.

2. For extented IndexOnlyScan - add support to avoid heap fetch in case of
OFFSET applied to tuple.

If first part is implemented - OFFSET optimisation is much easier to
achieve.

Thanks,
Michail.

Re: Define variable only in the scope that needs it

[Robert Haas]

On Tue, Mar 20, 2018 at 7:11 AM, Antonin Houska  wrote:
> I suppose the current definition in the higher scope is not intentional. This
> is what I suggest:

This is moot in light of commit 4f15e5d09de276fb77326be5567dd9796008ca2e.

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

Re: PATCH: Configurable file mode mask

[David Steele]

On 3/16/18 11:12 AM, Stephen Frost wrote:
> 
>>> Visibly there would be no need for a -g switch in
>>> pg_basebackup as it is possible to guess from the received untar'ed
>>> files what should be the permissions of the data based on what is
>>> received in pg_basebackup.c.  It would also be necessary to change the
>>> permissions of pg_wal as this is created before receiving any files.
>>
>> This part might be trickier.
> 
> This seems like another case where what we should be doing, and what
> people will be expecting, I'd think, is just what they're used to tar
> doing in these cases- which would be setting the dir/file mode for each
> file based on what's in the tarball.  Again, the files which are in the
> data dir are, sadly, not always just those that PG is familiar with.

I've been working on this and have become convinced that adding group
permissions to files that pg_basebackup writes to disk based on whether
group permissions are enabled in PGDATA isn't the right way to go.

To be clear, I'm not taking about the permissions set within the tar
file - I think it makes sense to use the actual PGDATA permissions in
that case.

pg_basebackup may not be running as postgres, and even if it is I don't
think we can assume that group access is appropriate for the files that
it writes.  It's a different environment and different security rules
may apply.

It seems to me that pg_basebackup and pg_receivexlog should have a -g
option to control the mode of the files that they write to disk (not
including the modes stored in the tar files).

Or perhaps we should just update the perms in the tar files for now and
leave the rest alone.

Thoughts?
-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: INOUT parameters in procedures

[Pavel Stehule]

2018-03-20 15:18 GMT+01:00 Merlin Moncure :

> On Tue, Mar 20, 2018 at 9:09 AM, Pavel Stehule 
> wrote:
> >> Edit: In one case, after dropping the function and recreating it, I
> >> got the procedure to return 0 where it had not before, so this smells
> >> like a bug.
> >> postgres=# call p();
> >> 2018-03-20 09:04:50.543 CDT [21494] ERROR:  function p() does not
> >> exist at character 6
> >> 2018-03-20 09:04:50.543 CDT [21494] HINT:  No function matches the
> >> given name and argument types. You might need to add explicit type
> >> casts.
> >> 2018-03-20 09:04:50.543 CDT [21494] STATEMENT:  call p();
> >> ERROR:  function p() does not exist
> >> LINE 1: call p();
> >>  ^
> >> HINT:  No function matches the given name and argument types. You
> >> might need to add explicit type casts.
> >> Time: 0.297 ms
> >> postgres=# create or replace procedure p(a inout int default 7) as $$
> >> begin return; end; $$ language plpgsql;
> >> CREATE PROCEDURE
> >> Time: 1.182 ms
> >> postgres=# call p();
> >>  a
> >> ───
> >>  0
> >> (1 row)
> >
> >
> > I wrote patch
>
> Confirmed this fixes the issue.
>

Thanks for info

Pavel


>
> merlin
>

Re: [PoC PATCH] Parallel dump to /dev/null

[Christoph Berg]

Re: Tom Lane 2018-03-20 <12960.1521557...@sss.pgh.pa.us>
> It might help if the patch were less enthusiastic about trying to
> "optimize" by avoiding extra file opens/closes in the case of output
> to /dev/null.  That seems to account for a lot of the additional
> complication, and I can't see a reason that it'd be worth it.

Note that the last patch was just a PoC to check if the extra
open/close could be avoided. The "real" patch is the 2nd last.

Mit freundlichen Grüßen,
Christoph Berg
-- 
Senior Berater, Tel.: +49 2166 9901 187
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
pgp fingerprint: 5C48 FE61 57F4 9179 5970  87C6 4C5A 6BAB 12D2 A7AE

Re: [PoC PATCH] Parallel dump to /dev/null

[Tom Lane]

Christoph Berg  writes:
> Re: Michael Paquier 2018-03-20 <20180320135720.ge13...@paquier.xyz>
>> Now, why are people using pg_dump > /dev/null?  Mainly the lack of
>> better tools, which would be actually able to detect pages in corrupted
>> pages in one run, and not only heap pages.  I honestly think that
>> amcheck is something that we sould more focus on and has more potential
>> on the matter, and that we are just complicating pg_dump to do something
>> it is not designed for, and would do it badly anyway.

FWIW, I've been wondering the same thing about this patch.  I hadn't
bothered to actually read the thread up to now, but the title was
enough to make me think "do we really need that?"

> Still, people are using it for that purpose now, and speeding it up
> would just be a non-intrusive patch.

If it were non-intrusive, that would be OK, but after a quick look
at the latest patch I can't say I find it so.  It seems to add a bunch
of poorly-defined additional states to each pg_dump format module.

It might help if the patch were less enthusiastic about trying to
"optimize" by avoiding extra file opens/closes in the case of output
to /dev/null.  That seems to account for a lot of the additional
complication, and I can't see a reason that it'd be worth it.

regards, tom lane

Re: file cloning in pg_upgrade and CREATE DATABASE

[Peter Eisentraut]

On 3/19/18 22:58, Michael Paquier wrote:
> I have been thinking about this patch over the night, and here is a list
> of bullet points which would be nice to tackle:
> - Remove the current diff in copydir.

done

> - Extend copy_file so as it is able to use fcopyfile.

fcopyfile() does not support cloning.  (This is not documented.)

> - Move the work done in pg_upgrade into a common API which can as well
> be used by pg_rewind as well.  One place would be to have a
> frontend-only API in src/common which does the leg work.  I would
> recommend working only on file descriptors as well for consistency with
> copy_file_range.

pg_upgrade copies files, whereas pg_rewind needs to copy file ranges.
So I don't think this is going to be a good match.

We could add support for using Linux copy_file_range() in pg_rewind, but
that would work a bit differently.  I also don't have a good sense of
how to test the performance of that.

Another thing to think about is that we go through some trouble to
initialize new WAL files so that the disk space is fully allocated.  If
we used file cloning calls in pg_rewind, that would potentially
invalidate some of that.  At least, we'd have to think through this more
carefully.

> - Add proper wait events for the backend calls.  Those are missing for
> copy_file_range and copyfile.

done

> - For XLogFileCopy, the problem may be trickier as the tail of a segment
> is filled with zeroes, so dropping it from the first version of the
> patch sounds wiser.

Seems like a possible follow-on project.  But see also under pg_rewind
above.

Another oddity is that pg_upgrade uses CopyFile() on Windows, but the
backend does not.  The Git log shows that the backend used to use
CopyFile(), but that was then removed when the generic code was added,
but when pg_upgrade was imported, it came with the CopyFile() call.

I suspect the CopyFile() call can be quite a bit faster, so we should
consider adding it back in.  Or if not, remove it from pg_upgrade.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From bd8fe105f6b1c64098e344c4a7d0fc9c94d2e31d Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 20 Mar 2018 10:21:47 -0400
Subject: [PATCH v2] Use file cloning in pg_upgrade and CREATE DATABASE

For file copying in pg_upgrade and CREATE DATABASE, use special file
cloning calls if available.  This makes the copying faster and more
space efficient.  For pg_upgrade, this achieves speed similar to --link
mode without the associated drawbacks.  Other backend users of copydir.c
will also take advantage of these changes, but the performance
improvement will probably not be as noticeable there.

On Linux, use copy_file_range().  This supports file cloning
automatically on Btrfs and XFS (if formatted with reflink support).  On
macOS, use copyfile(), which supports file cloning on APFS.

Even on file systems without cloning/reflink support, this is faster
than the existing code, because it avoids copying the file contents out
of kernel space and allows the OS to apply other optimizations.
---
 configure  |  2 +-
 configure.in   |  2 +-
 doc/src/sgml/monitoring.sgml   |  8 +++-
 doc/src/sgml/ref/pgupgrade.sgml| 11 +
 src/backend/postmaster/pgstat.c|  3 ++
 src/backend/storage/file/copydir.c | 84 ++
 src/backend/storage/file/reinit.c  |  3 +-
 src/bin/pg_upgrade/file.c  | 56 +++--
 src/include/pg_config.h.in |  6 +++
 src/include/pgstat.h   |  1 +
 10 files changed, 141 insertions(+), 35 deletions(-)

diff --git a/configure b/configure
index 3943711283..f27c78f63a 100755
--- a/configure
+++ b/configure
@@ -13085,7 +13085,7 @@ fi
 LIBS_including_readline="$LIBS"
 LIBS=`echo "$LIBS" | sed -e 's/-ledit//g' -e 's/-lreadline//g'`
 
-for ac_func in cbrt clock_gettime dlopen fdatasync getifaddrs getpeerucred 
getrlimit mbstowcs_l memmove poll posix_fallocate pstat pthread_is_threaded_np 
readlink setproctitle setsid shm_open symlink sync_file_range utime utimes 
wcstombs_l
+for ac_func in cbrt clock_gettime copy_file_range copyfile dlopen fdatasync 
getifaddrs getpeerucred getrlimit mbstowcs_l memmove poll posix_fallocate pstat 
pthread_is_threaded_np readlink setproctitle setsid shm_open symlink 
sync_file_range utime utimes wcstombs_l
 do :
   as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
 ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
diff --git a/configure.in b/configure.in
index 1babdbb755..7eb8673753 100644
--- a/configure.in
+++ b/configure.in
@@ -1428,7 +1428,7 @@ PGAC_FUNC_WCSTOMBS_L
 LIBS_including_readline="$LIBS"
 LIBS=`echo "$LIBS" | sed -e 's/-ledit//g' -e 's/-lreadline//g'`
 
-AC_CHECK_FUNCS([cbrt clock_gettime dlopen fdatasync getifaddrs getpeerucred 
getrlimit mbstowcs_l memmove poll posix_fallocate pstat pthread_is_threaded_np 
readlink 

Re: [HACKERS] Partition-wise aggregation/grouping

[Jeevan Chalke]

Hi,

On Mon, Mar 19, 2018 at 10:56 PM, Robert Haas  wrote:

> On Fri, Mar 16, 2018 at 1:50 PM, Ashutosh Bapat
>  wrote:
> > Ok. That looks good.
>
> Here's an updated version.  In this version, based on a voice
> discussion with Ashutosh and Jeevan, I adjusted 0001 to combine it
> with an earlier idea of splitting Gather/Gather Merge path generation
> out of the function that creates partially aggregated paths.  The idea
> here is that create_ordinary_gather_paths() could first call
> create_partial_grouping_paths(), then add additional paths which might
> be partial or non-partial by invoking the partition-wise aggregate
> logic, then call gather_grouping_paths() and set_cheapest() to
> finalize the partially grouped rel.  Also, I added draft commit
> messages.
>

I have added all these three patches in the attached patch-set and rebased
my changes over it.

However, I have not yet made this patch-set dependednt on UPPERREL_TLIST
changes you have proposed on another mail-thread and thus it has 0001 patch
refactoring the scanjoin issue.
0002, 0003 and 0004 are your patches added in this patchset.
0005 and 0006 are further refactoring patches. 0006 adds a
GroupPathExtraData which stores mostly child variant data and costs.
0007 is main partitionwise aggregation patch which is then rebased
accordingly.
0008 contains testcase and 0009 contains FDW changes.

Let me know if I missed any point to consider while rebasing.

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


partition-wise-agg-v21.tar.gz
Description: application/gzip

Re: XID-assigned idle transactions affect vacuum's job.

[Masahiko Sawada]

Hi, Amit

On Tue, Mar 20, 2018 at 8:02 PM, Amit Kapila  wrote:
> On Tue, Mar 20, 2018 at 12:22 PM, Masahiko Sawada  
> wrote:
>> Hi,
>>
>> Long transactions often annoy users because if a long transaction
>> exists on a database vacuum cannot reclaim efficiently. There are
>> several reason why they exist on a database but it's a common case
>> where users or applications forget to commit/rollback transactions.
>> That is, transaction is not executing SQL and its state is 'idle in
>> transaction' on pg_stat_activity. In this case, such transactions
>> don't affect vacuum's job either if they aren't assigned transaction
>> id or if they don't have a snapshot. However if they have xid it will
>> affect vacuum's job even if they don't have a snapshot.
>>
>> I think that to decide which deleted tuples must be preserved we don't
>> need to care about backend PGXACT.xid but must care about PGXACT.xmin.
>> But current GetOldestXmin considers both of them. I guess one reason
>> why GetOldestXmin does so is that it's also used to determine where to
>> truncate pg_subtrans. Is there anything else reason?
>>
>
> I think the main reason is that while computing snapshots, we also
> rely on PGXACT.xid.  Basically, it can be present in some other
> snapshots xmin.  Now, if you ignore it in vacuum (GetOldestXmin), then
> it is quite possible that the xid we have ignored will be part of some
> other snapshot's xmin which I think in turn can lead to wrong results.
>

Sorry I'm still confusing. You meant that it's possible that an xmin
of a snapshot can be older than the oldest PGXACT.xmin? If it's
possible I'm sure the problem happens but I'm not sure it can happen
because PGXACT.xmin is the oldest xid when taking a snapshot. I think
that the oldest PGXACT.xmin can be either the same as or younger than
the oldest PGXACT.xid.

Regards,

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

Re: Lack of T_TargetEntry in exprType function

[Tom Lane]

Konstantin Knizhnik  writes:
> On 20.03.2018 17:00, Tom Lane wrote:
>> It's intentional because that's not considered an executable
>> expression.

> I tried to apply this function to the argument of Aggref and it has 
> exactly this kind.
> Aggref is executable expression. This is why I thought that all its 
> arguments also should be considered as executable expressions, shouldn't 
> they?

Not really.  They're a list of executable expressions, which is not the
same thing --- in particular, exprType() on the whole list wouldn't give a
useful result.  The TargetEntrys are best thought of as part of the list
superstructure.

regards, tom lane

Re: configure's checks for --enable-tap-tests are insufficient

[Tom Lane]

ilm...@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
> Tom Lane  writes:
>> Ah-hah.  So it seems to me that it'd be a useful exercise to start
>> from perl-base and find out what has to be added to get to working
>> TAP tests.  Anyone?

> I think the other way would be better simpler: we require a complete
> Perl core installation.

And you propose to test for that in configure how?

regards, tom lane

Re: Online enabling of checksums

[Magnus Hagander]

On Mon, Mar 19, 2018 at 7:27 AM, Heikki Linnakangas  wrote:

> Hi,
>
> The patch looks good to me at a high level. Some notes below. I didn't
> read through the whole thread, so sorry if some of these have been
> discussed already.
>
> +void
>> +ShutdownChecksumHelperIfRunning(void)
>> +{
>> +   if (pg_atomic_unlocked_test_flag(>launcher
>> _started))
>> +   {
>> +   /* Launcher not started, so nothing to shut down */
>> +   return;
>> +   }
>> +
>> +   ereport(ERROR,
>> +   (errmsg("checksum helper is currently running,
>> cannot disable checksums"),
>> +errhint("Restart the cluster or wait for the
>> worker to finish.")));
>> +}
>>
>
> Is there no way to stop the checksum helper once it's started? That seems
> rather user-unfriendly. I can imagine it being a pretty common mistake to
> call pg_enable_data_checksums() on a 10 TB cluster, only to realize that
> you forgot to set the cost limit, and that it's hurting queries too much.
> At that point, you want to abort.
>

Agreed. You could do it with pg_terminate_backend() but you had to do it in
the right order, etc.

Attached patch fixes this by making it possible to abort the process by
executing pg_disable_data_checksums() during the process. In this case the
live workers will abort, and the checksums will be switched off again.



> +extern bool DataChecksumsEnabledOrInProgress(void);
>> +extern bool DataChecksumsInProgress(void);
>> +extern bool DataChecksumsDisabled(void);
>>
>
> I find the name of the DataChecksumsEnabledOrInProgress() function a bit
> long. And doing this in PageIsVerified looks a bit weird:
>
> > if (DataChecksumsEnabledOrInProgress() && !DataChecksumsInProgress())
>
> I think I'd prefer functions like:
>
> /* checksums should be computed on write? */
> bool DataChecksumNeedWrite()
> /* checksum should be verified on read? */
> bool DataChecksumNeedVerify()
>

Agreed. We also need DataChecksumsInProgress() to make it work properly,
but that makes the names a lot more clear. Adjusted as such.



> + template0 is by default not accepting
>> connections, to
>> + enable checksums you'll need to temporarily make it accept
>> connections.
>>
>
> This was already discussed, and I agree with the other comments that it's
> bad.
>

Do you have any opinion on the thread about how to handle that one? As in
which way to go about solving it? (The second thread linked from the CF
entry - it wasn't linked before as intended, but it is now)



+CREATE OR REPLACE FUNCTION pg_enable_data_checksums (
>> +cost_delay int DEFAULT 0, cost_limit int DEFAULT 100)
>> +  RETURNS boolean STRICT VOLATILE LANGUAGE internal AS
>> 'enable_data_checksums'
>> +  PARALLEL RESTRICTED;
>>
>
> pg_[enable|disable]_checksums() functions return a bool. It's not clear
> to me when they would return what. I'd suggest marking them as 'void'
> instead.


Agreed, changed.


--- a/src/include/storage/bufpage.h
>> +++ b/src/include/storage/bufpage.h
>> @@ -194,6 +194,7 @@ typedef PageHeaderData *PageHeader;
>>   */
>>  #define PG_PAGE_LAYOUT_VERSION 4
>>  #define PG_DATA_CHECKSUM_VERSION   1
>> +#define PG_DATA_CHECKSUM_INPROGRESS_VERSION2
>>
>>
>
> This seems like a weird place for these PG_DATA_CHECKSUM_* constants.
> They're not actually stored in the page header, as you might think. I think
> the idea was to keep PG_DATA_CHECKSUM_VERSION close to
> PG_PAGE_LAYOUT_VERSION, because the checksums affected the on-disk format.
> I think it was a bit weird even before this patch, but now it's worse. At
> least a better comment would be in order, or maybe move these elsewhere.
>

True, but they apply to the page level? I'm not sure about the original
reasoning to put them there,figured it wasn't the responsibility of this
patch to move them. But we can certainly do that.

But what would be an appropriate place elsewhere? First thought would be
pg_control.h, but that would then not be consistent with e.g. wal_level
(which is declared in xlog.h and not pg_control.h..


Feature request: it'd be nice to update the 'ps status' with
> set_ps_display, to show a rudimentary progress indicator. Like the name and
> block number of the relation being processed. It won't tell you how much is
> left to go, but at least it will give a warm fuzzy feeling to the DBA that
> something is happening.
>

In the attached patch it sets this information in pg_stat_activity. I think
that makes more sense than the ps display, and I think is more consistent
with other ways we use them (e.g. autovacuum doesn't update it's ps title
for every table, but it does update pg_stat_activity).


I didn't review the offline tool, pg_verify_checksums.


PFA a patch that takes into account these comments and we believe all other
pending ones as well.


-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: XID-assigned idle transactions affect vacuum's job.

[Tom Lane]

Masahiko Sawada  writes:
> I think that to decide which deleted tuples must be preserved we don't
> need to care about backend PGXACT.xid but must care about PGXACT.xmin.

Surely this is wrong?  Assume that the XID in question is the oldest one
visible in the ProcArray (if it isn't, the question is moot anyway).
If we ignore it while setting VACUUM's cutoff, then if we come to a
tuple bearing that XID, we will think the transaction involved is
aborted (since it's not marked as committed) and proceed to remove the
tuple.

As Amit remarks, the fact that that XID will constrain the XMINs of other
open transactions means you'd usually not get any win anyway.  But AFAICS,
this proposal is incorrect even without that.

regards, tom lane

Re: [PoC PATCH] Parallel dump to /dev/null

[Christoph Berg]

Re: Michael Paquier 2018-03-20 <20180320135720.ge13...@paquier.xyz>
> Now, why are people using pg_dump > /dev/null?  Mainly the lack of
> better tools, which would be actually able to detect pages in corrupted
> pages in one run, and not only heap pages.  I honestly think that
> amcheck is something that we sould more focus on and has more potential
> on the matter, and that we are just complicating pg_dump to do something
> it is not designed for, and would do it badly anyway.

Still, people are using it for that purpose now, and speeding it up
would just be a non-intrusive patch.

Also, if pg_dump is a backup tool, why does that mean it must not be
used for anything else?

Mit freundlichen Grüßen,
Christoph Berg
-- 
Senior Berater, Tel.: +49 2166 9901 187
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
pgp fingerprint: 5C48 FE61 57F4 9179 5970  87C6 4C5A 6BAB 12D2 A7AE

Re: configure's checks for --enable-tap-tests are insufficient

[Dagfinn Ilmari Mannsåker]

Tom Lane  writes:

> ilm...@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
>> Note that Fedora aren't the only ones to do this: Debian also splits the
>> Perl core dist into multiple packages.  The crucial difference is that
>> on Debian installing the 'perl' package gives you the whole lot, while
>> on Fedora that's just a subset (similar to Debian's 'perl-base'
>> package); you have to install 'perl-core' to get everything.
>
> Ah-hah.  So it seems to me that it'd be a useful exercise to start
> from perl-base and find out what has to be added to get to working
> TAP tests.  Anyone?

I disagree.  The set of modules in perl-base is not guaranteed to stay
the same across Debian releases: it's just whatever the Debian base
system needs.  Similarly, the split of modules between the perl and
perl-core (and even indvidual perl-Foo-Bar) packages has varied between
Fedora/RHEL releases.

I think the other way would be better simpler: we require a complete
Perl core installation.  This means that on Fedora < 27 and RHEL < 8 you
need to install perl-core, not perl.

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

Re: Lack of T_TargetEntry in exprType function

[Konstantin Knizhnik]

On 20.03.2018 17:00, Tom Lane wrote:

Konstantin Knizhnik  writes:

Is there any reason for not handling T_TargetEntry node kind in
exprType() function in nodeFuncs.c?

It's intentional because that's not considered an executable
expression.


I tried to apply this function to the argument of Aggref and it has 
exactly this kind.
Aggref is executable expression. This is why I thought that all its 
arguments also should be considered as executable expressions, shouldn't 
they?
My idea was to capture argument of one aggregate and use  it in another 
aggregate during query transformation.
Certainly it is not a problem for me to handle this case myself, but I 
just wonder will it be more natural to extend exprType to handle this 
node kind as well.





But why in this case it is handled in the exprLocation function?

There are lots of things that have syntactic locations but aren't
value-yielding expressions.

regards, tom lane


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

Re: INOUT parameters in procedures

[Merlin Moncure]

On Tue, Mar 20, 2018 at 9:09 AM, Pavel Stehule  wrote:
>> Edit: In one case, after dropping the function and recreating it, I
>> got the procedure to return 0 where it had not before, so this smells
>> like a bug.
>> postgres=# call p();
>> 2018-03-20 09:04:50.543 CDT [21494] ERROR:  function p() does not
>> exist at character 6
>> 2018-03-20 09:04:50.543 CDT [21494] HINT:  No function matches the
>> given name and argument types. You might need to add explicit type
>> casts.
>> 2018-03-20 09:04:50.543 CDT [21494] STATEMENT:  call p();
>> ERROR:  function p() does not exist
>> LINE 1: call p();
>>  ^
>> HINT:  No function matches the given name and argument types. You
>> might need to add explicit type casts.
>> Time: 0.297 ms
>> postgres=# create or replace procedure p(a inout int default 7) as $$
>> begin return; end; $$ language plpgsql;
>> CREATE PROCEDURE
>> Time: 1.182 ms
>> postgres=# call p();
>>  a
>> ───
>>  0
>> (1 row)
>
>
> I wrote patch

Confirmed this fixes the issue.

merlin

Re: INOUT parameters in procedures

[Pavel Stehule]

2018-03-20 15:05 GMT+01:00 Merlin Moncure :

> On Wed, Feb 28, 2018 at 4:28 PM, Peter Eisentraut
>  wrote:
> > This patch set adds support for INOUT parameters to procedures.
> > Currently, INOUT and OUT parameters are not supported.
> >
> > A top-level CALL returns the output parameters as a result row.  In
> > PL/pgSQL, I have added special support to pass the output back into the
> > variables, as one would expect.
> >
> > These patches apply on top of the "prokind" patch set v2.  (Tom has
> > submitted an updated version of that, which overlaps with some of the
> > changes I've made here.  I will work on consolidating that soon.)
>
> I did a pull from master to play around with INOUT parameters and got
> some strange interactions with DEFAULT.  Specifically, DEFAULT doesn't
> do much beyond, 'return the last supplied value given'.  I'm not sure
> if this is expected behavior; it seems odd:
>
> postgres=# create or replace procedure p(a inout int default 7) as $$
> begin return; end; $$ language plpgsql;
> CREATE PROCEDURE
> postgres=# call p();
>  a
> ───
>
> (1 row)
>
> postgres=# call p(3);
>  a
> ───
>  3
> (1 row)
>
> postgres=# call p();
>  a
> ───
>  3
> (1 row)
>
>
> I got null,3,3.  I would have expected 7,3,7.  Default arguments might
> remove quite some of the pain associated with having to supply bogus
> arguments to get the INOUT parameters working.
>
> Edit: In one case, after dropping the function and recreating it, I
> got the procedure to return 0 where it had not before, so this smells
> like a bug.
> postgres=# call p();
> 2018-03-20 09:04:50.543 CDT [21494] ERROR:  function p() does not
> exist at character 6
> 2018-03-20 09:04:50.543 CDT [21494] HINT:  No function matches the
> given name and argument types. You might need to add explicit type
> casts.
> 2018-03-20 09:04:50.543 CDT [21494] STATEMENT:  call p();
> ERROR:  function p() does not exist
> LINE 1: call p();
>  ^
> HINT:  No function matches the given name and argument types. You
> might need to add explicit type casts.
> Time: 0.297 ms
> postgres=# create or replace procedure p(a inout int default 7) as $$
> begin return; end; $$ language plpgsql;
> CREATE PROCEDURE
> Time: 1.182 ms
> postgres=# call p();
>  a
> ───
>  0
> (1 row)
>

I wrote patch

Regards

Pavel


>
>
> merlin
>
>
diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c
index 86fa8c0dd7..c7a44d858b 100644
--- a/src/backend/commands/functioncmds.c
+++ b/src/backend/commands/functioncmds.c
@@ -55,6 +55,7 @@
 #include "executor/executor.h"
 #include "miscadmin.h"
 #include "optimizer/var.h"
+#include "optimizer/clauses.h"
 #include "parser/parse_coerce.h"
 #include "parser/parse_collate.h"
 #include "parser/parse_expr.h"
@@ -2254,6 +2255,9 @@ ExecuteCallStmt(CallStmt *stmt, ParamListInfo params, bool atomic, DestReceiver
 		elog(ERROR, "cache lookup failed for function %u", fexpr->funcid);
 	if (!heap_attisnull(tp, Anum_pg_proc_proconfig))
 		callcontext->atomic = true;
+
+	fexpr->args = expand_function_arguments(fexpr->args, fexpr->funcresulttype, tp);
+
 	ReleaseSysCache(tp);
 
 	/* Initialize function call structure */
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index a9a09afd2b..40eae3a835 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -130,8 +130,6 @@ static Expr *simplify_function(Oid funcid,
   Oid result_collid, Oid input_collid, List **args_p,
   bool funcvariadic, bool process_args, bool allow_non_const,
   eval_const_expressions_context *context);
-static List *expand_function_arguments(List *args, Oid result_type,
-		  HeapTuple func_tuple);
 static List *reorder_function_arguments(List *args, HeapTuple func_tuple);
 static List *add_function_defaults(List *args, HeapTuple func_tuple);
 static List *fetch_function_defaults(HeapTuple func_tuple);
@@ -4112,7 +4110,7 @@ simplify_function(Oid funcid, Oid result_type, int32 result_typmod,
  * cases it handles should never occur there.  This should be OK since it
  * will fall through very quickly if there's nothing to do.
  */
-static List *
+List *
 expand_function_arguments(List *args, Oid result_type, HeapTuple func_tuple)
 {
 	Form_pg_proc funcform = (Form_pg_proc) GETSTRUCT(func_tuple);
diff --git a/src/include/optimizer/clauses.h b/src/include/optimizer/clauses.h
index ba4fa4b68b..ed854fdd40 100644
--- a/src/include/optimizer/clauses.h
+++ b/src/include/optimizer/clauses.h
@@ -14,9 +14,9 @@
 #ifndef CLAUSES_H
 #define CLAUSES_H
 
+#include "access/htup.h"
 #include "nodes/relation.h"
 
-
 #define is_opclause(clause)		((clause) != NULL && IsA(clause, OpExpr))
 #define is_funcclause(clause)	((clause) != NULL && IsA(clause, FuncExpr))
 
@@ -85,4 +85,7 @@ extern Node *estimate_expression_value(PlannerInfo *root, Node *node);
 extern Query *inline_set_returning_function(PlannerInfo *root,
 			

Re: INOUT parameters in procedures

[Merlin Moncure]

On Wed, Feb 28, 2018 at 4:28 PM, Peter Eisentraut
 wrote:
> This patch set adds support for INOUT parameters to procedures.
> Currently, INOUT and OUT parameters are not supported.
>
> A top-level CALL returns the output parameters as a result row.  In
> PL/pgSQL, I have added special support to pass the output back into the
> variables, as one would expect.
>
> These patches apply on top of the "prokind" patch set v2.  (Tom has
> submitted an updated version of that, which overlaps with some of the
> changes I've made here.  I will work on consolidating that soon.)

I did a pull from master to play around with INOUT parameters and got
some strange interactions with DEFAULT.  Specifically, DEFAULT doesn't
do much beyond, 'return the last supplied value given'.  I'm not sure
if this is expected behavior; it seems odd:

postgres=# create or replace procedure p(a inout int default 7) as $$
begin return; end; $$ language plpgsql;
CREATE PROCEDURE
postgres=# call p();
 a
───

(1 row)

postgres=# call p(3);
 a
───
 3
(1 row)

postgres=# call p();
 a
───
 3
(1 row)


I got null,3,3.  I would have expected 7,3,7.  Default arguments might
remove quite some of the pain associated with having to supply bogus
arguments to get the INOUT parameters working.

Edit: In one case, after dropping the function and recreating it, I
got the procedure to return 0 where it had not before, so this smells
like a bug.
postgres=# call p();
2018-03-20 09:04:50.543 CDT [21494] ERROR:  function p() does not
exist at character 6
2018-03-20 09:04:50.543 CDT [21494] HINT:  No function matches the
given name and argument types. You might need to add explicit type
casts.
2018-03-20 09:04:50.543 CDT [21494] STATEMENT:  call p();
ERROR:  function p() does not exist
LINE 1: call p();
 ^
HINT:  No function matches the given name and argument types. You
might need to add explicit type casts.
Time: 0.297 ms
postgres=# create or replace procedure p(a inout int default 7) as $$
begin return; end; $$ language plpgsql;
CREATE PROCEDURE
Time: 1.182 ms
postgres=# call p();
 a
───
 0
(1 row)


merlin

Re: Lack of T_TargetEntry in exprType function

[Tom Lane]

Konstantin Knizhnik  writes:
> Is there any reason for not handling T_TargetEntry node kind in 
> exprType() function in nodeFuncs.c?

It's intentional because that's not considered an executable
expression.

> But why in this case it is handled in the exprLocation function?

There are lots of things that have syntactic locations but aren't
value-yielding expressions.

regards, tom lane

Re: [PoC PATCH] Parallel dump to /dev/null

[Michael Paquier]

On Tue, Mar 20, 2018 at 09:54:07AM +0100, Christoph Berg wrote:
> Otherwise, +1 from me.

I have been thinking about this patch lately, and on the contrary I am
voting -1 for the concepts behind this patch.  pg_dump is by nature a
tool aimed at fetching data from the database, at putting it in a shape 
wanted by the user, and optionally at saving the data and at making it
durable (since recently for the last part). 
It is not a corruption detection tool.  Even if it was a tool to detect
corruption, it is doing it wrong in two ways:
1) It scans tables using only sequential scans, so it basically never
checks any other AMs than heap.
2) It detects only one failure at a time and stops.  Hence in order to
detect all corruptions, one need to run pg_dump, repair or zero the
pages and then rince and repeat until a successful run is achieved.
This is a costly process particularly on large relations, where a run of
pg_dump can take minutes, and the more the pages, the more time it takes
to do the whole cleanup before being able to save as much data as
possible.

Now, why are people using pg_dump > /dev/null?  Mainly the lack of
better tools, which would be actually able to detect pages in corrupted
pages in one run, and not only heap pages.  I honestly think that
amcheck is something that we sould more focus on and has more potential
on the matter, and that we are just complicating pg_dump to do something
it is not designed for, and would do it badly anyway.
--
Michael


signature.asc
Description: PGP signature

Re: configure's checks for --enable-tap-tests are insufficient

[Tom Lane]

ilm...@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
> Note that Fedora aren't the only ones to do this: Debian also splits the
> Perl core dist into multiple packages.  The crucial difference is that
> on Debian installing the 'perl' package gives you the whole lot, while
> on Fedora that's just a subset (similar to Debian's 'perl-base'
> package); you have to install 'perl-core' to get everything.

Ah-hah.  So it seems to me that it'd be a useful exercise to start
from perl-base and find out what has to be added to get to working
TAP tests.  Anyone?

regards, tom lane

Re: [PROPOSAL] Shared Ispell dictionaries

[Arthur Zakirov]

Hello,

On Mon, Mar 19, 2018 at 08:50:46PM +0100, Tomas Vondra wrote:
> Hi Arthur,
> 
> I went through the patch - just skimming through the diffs, will do more
> testing tomorrow. Here are a few initial comments.

Thank you for the review!

> 1) max_shared_dictionaries_size / PGC_POSTMASTER
> 
> I'm not quite sure why the GUC is defined as PGC_POSTMASTER, i.e. why it
> can't be changed after server start. That seems like a fairly useful
> thing to do (e.g. increase the limit while the server is running), and
> after looking at the code I think it shouldn't be difficult to change.

max_shared_dictionaries_size is defined as PGC_SIGHUP now. Added check
of a new value to disallow to set zero if there are loaded dictionaries
and to decrease maximum allowed size if loaded size is greater than the
new value.

> The other thing I'd suggest is handling "-1" as "no limit".

I added availability to set '-1'. Fixed some comments and the
documentation.

> 2) max_shared_dictionaries_size / size of number
> 
> Some of the comments dealing with the GUC treat it as a number of
> dictionaries (instead of a size). I suppose that's due to how the
> original patch was implemented.

Fixed. Should be good now.

> 3) Assert(max_shared_dictionaries_size);
> 
> I'd say that assert is not very clear - it should be
> 
> Assert(max_shared_dictionaries_size > 0);
> 
> or something along the lines. It's also a good idea to add a comment
> explaining the assert, say
> 
> /* we can only get here when shared dictionaries are enabled */
> Assert(max_shared_dictionaries_size > 0);

Fixed the assert and added the comment. I extended the assert, it also
takes into account -1 value.

> 4) I took the liberty of rewording some of the docs/comments. See the
> attached diffs, that should apply on top of 0003 and 0004 patches.
> Please, treat those as mere suggestions.

I applied your diffs and added changes to max_shared_dictionaries_size.

Please find the attached new version of the patch.

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
diff --git a/src/backend/tsearch/spell.c b/src/backend/tsearch/spell.c
index b9fdd77e19..e071994523 100644
--- a/src/backend/tsearch/spell.c
+++ b/src/backend/tsearch/spell.c
@@ -78,6 +78,8 @@
 #define tmpalloc(sz)  MemoryContextAlloc(Conf->buildCxt, (sz))
 #define tmpalloc0(sz)  MemoryContextAllocZero(Conf->buildCxt, (sz))
 
+#define tmpstrdup(str) MemoryContextStrdup(Conf->buildCxt, (str))
+
 /*
  * Prepare for constructing an ISpell dictionary.
  *
@@ -498,7 +500,7 @@ NIAddSpell(IspellDict *Conf, const char *word, const char 
*flag)
Conf->Spell[Conf->nspell] = (SPELL *) tmpalloc(SPELLHDRSZ + 
strlen(word) + 1);
strcpy(Conf->Spell[Conf->nspell]->word, word);
Conf->Spell[Conf->nspell]->p.flag = (*flag != '\0')
-   ? cpstrdup(Conf, flag) : VoidString;
+   ? tmpstrdup(flag) : VoidString;
Conf->nspell++;
 }
 
@@ -1040,7 +1042,7 @@ setCompoundAffixFlagValue(IspellDict *Conf, 
CompoundAffixFlag *entry,
entry->flag.i = i;
}
else
-   entry->flag.s = cpstrdup(Conf, s);
+   entry->flag.s = tmpstrdup(s);
 
entry->flagMode = Conf->flagMode;
entry->value = val;
@@ -1536,6 +1538,9 @@ nextline:
return;
 
 isnewformat:
+   pfree(recoded);
+   pfree(pstr);
+
if (oldformat)
ereport(ERROR,
(errcode(ERRCODE_CONFIG_FILE_ERROR),
diff --git a/contrib/dict_int/dict_int.c b/contrib/dict_int/dict_int.c
index 56ede37089..e11d1129e9 100644
--- a/contrib/dict_int/dict_int.c
+++ b/contrib/dict_int/dict_int.c
@@ -30,7 +30,7 @@ PG_FUNCTION_INFO_V1(dintdict_lexize);
 Datum
 dintdict_init(PG_FUNCTION_ARGS)
 {
-   List   *dictoptions = (List *) PG_GETARG_POINTER(0);
+   DictInitData *init_data = (DictInitData *) PG_GETARG_POINTER(0);
DictInt*d;
ListCell   *l;
 
@@ -38,7 +38,7 @@ dintdict_init(PG_FUNCTION_ARGS)
d->maxlen = 6;
d->rejectlong = false;
 
-   foreach(l, dictoptions)
+   foreach(l, init_data->dictoptions)
{
DefElem*defel = (DefElem *) lfirst(l);
 
diff --git a/contrib/dict_xsyn/dict_xsyn.c b/contrib/dict_xsyn/dict_xsyn.c
index a79ece240c..c3146bae3c 100644
--- a/contrib/dict_xsyn/dict_xsyn.c
+++ b/contrib/dict_xsyn/dict_xsyn.c
@@ -140,7 +140,7 @@ read_dictionary(DictSyn *d, const char *filename)
 Datum
 dxsyn_init(PG_FUNCTION_ARGS)
 {
-   List   *dictoptions = (List *) PG_GETARG_POINTER(0);
+   DictInitData *init_data = (DictInitData *) PG_GETARG_POINTER(0);
DictSyn*d;
ListCell   *l;
char   *filename = NULL;
@@ -153,7 +153,7 @@ dxsyn_init(PG_FUNCTION_ARGS)
d->matchsynonyms = false;
d->keepsynonyms = true;
 
-   foreach(l, dictoptions)
+   foreach(l, init_data->dictoptions)
{
DefElem*defel = (DefElem *) 

[no subject]

[Torsten Grust]

G'day all,

what is the recommended method to dump the steps[] array of EvalSteps for a
compiled expression, showing the opcodes themselves as well as
details/arguments for the various opcodes? (I am asking out of curiosity
and for teaching purposes.) This must have been done plenty of time
already, I guess.

Thank you and best wishes,

—Torsten
-- 
| Prof. Dr. Torsten Grust
| Database Systems — Universität Tübingen (Germany)
| ✉︎ torsten.gr...@uni-tuebingen.de
| db.inf.uni-tuebingen.de

Re: [HACKERS] plpgsql - additional extra checks

[Tels]

Hello Pavel and Tomas,

On Tue, March 20, 2018 12:36 am, Pavel Stehule wrote:
> 2018-03-19 21:47 GMT+01:00 Tomas Vondra :
>
>> Hi,
>>
>> I'm looking at the updated patch (plpgsql-extra-check-180316.patch), and
>> this time it applies and builds OK. The one thing I noticed is that the
>> documentation still uses the old wording for strict_multi_assignement:
>>
>> WARNING:  Number of evaluated fields does not match expected.
>> HINT:  strict_multi_assignement check of extra_warnings is active.
>> WARNING:  Number of evaluated fields does not match expected.
>> HINT:  strict_multi_assignement check of extra_warnings is active.
>>
>> This was reworded to "Number of source and target fields in assignment
>> does not match."
>>

I believe the correct wording should be:

"Number of source and target fields in assignment do not match."

ecause comparing one number to the other means "the number A and B _do_
not match", not "the number A does not match number B".

Also there is an inconsistent quoting here:

+   
+Setting plpgsql.extra_warnings, or
+plpgsql.extra_errors, as appropriate, to
all

no quotes for 'all'.

+is encouraged in development and/or testing environments.
+   
+
+   
+These additional checks are enabled through the configuration variables
+plpgsql.extra_warnings for warnings and
+plpgsql.extra_errors for errors. Both can be set
either to
+a comma-separated list of checks, "none" or
+"all".

quotes here around '"all"'. I think it should be one or the other in both
cases.

Also:

+ Currently
+the list of available checks includes only one:

but then it lists more than one check?

Best wishes,

Tels

Re: [HACKERS] Optional message to user when terminating/cancelling backend

[Christoph Berg]

Re: Eren Başak 2018-03-20 

> Another thing is that, in a similar manner, we could allow changing the
> error code which might be useful for extensions. For example, Citus could
> use it to cancel remote backends when it detects a distributed deadlock and
> changes the error code to something retryable while doing so.

Another useful thing to do on top of this patch would be to include
messages when the termination comes from postgres itself, e.g. on a
server shutdown. Possibly, the message for pg_terminate_backend() itself could
say that someone invoke that, unless overridden.

FATAL:  57P01: terminating connection due to administrator command: server 
shutting down
FATAL:  57P01: terminating connection due to administrator command: restarting 
because of a crash of another server process
FATAL:  57P01: terminating connection due to administrator command: terminated 
by pg_terminate_backend()

Christoph

Re: [HACKERS] path toward faster partition pruning

[David Rowley]

On 21 March 2018 at 00:07, Amit Langote  wrote:
> Attached is further revised version.

In the 0004 patch I see:

@@ -1439,6 +1441,10 @@ inheritance_planner(PlannerInfo *root)
  if (IS_DUMMY_PATH(subpath))
  continue;

+ /* Add the current parent's RT index to the partitioned rels set. */
+ partitioned_relids = bms_add_member(partitioned_relids,
+ appinfo->parent_relid);

This seems to execute regardless of if the target relation is a
partitioned table or an inheritance parent. I think there needs to be
a condition so you only do this when planning for partitioned tables.

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

Re: [HACKERS] plpgsql - additional extra checks

[Tomas Vondra]

On 03/20/2018 05:36 AM, Pavel Stehule wrote:
> 
> 
> 2018-03-19 21:47 GMT+01:00 Tomas Vondra  >:
> 
> Hi,
> 
> I'm looking at the updated patch (plpgsql-extra-check-180316.patch), and
> this time it applies and builds OK. The one thing I noticed is that the
> documentation still uses the old wording for strict_multi_assignement:
> 
> WARNING:  Number of evaluated fields does not match expected.
> HINT:  strict_multi_assignement check of extra_warnings is active.
> WARNING:  Number of evaluated fields does not match expected.
> HINT:  strict_multi_assignement check of extra_warnings is active.
> 
> This was reworded to "Number of source and target fields in assignment
> does not match."
> 
> 
> fixed
> 

OK, thanks. PFA I've marked it as ready for committer.

I think the patch is solid code-wise, but I'm sure it might benefit e.g.
from a native speaker checking the wording of comments and messages.

regards

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

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Alvaro Herrera]

Hi,

Amit Langote wrote:

> 2. If I understand the description you provided in [1] correctly, the
> point of having ri_PartitionIsValid and ExecInitExtraPartitionInfo() is to
> avoid possibly-redundantly performing following two steps in
> ExecSetupPartitionTupleRouting() for *reused* partition ResultRelInfos
> that may not be used for tuple routing after all:
> 
>  - create the parent_child_tupconv_maps[] entry for it
>  - perform FDW tuple routing initialization.
> 
> If that's true, the following comment could be expanded just a little bit
> to make that clearer:
> 
> /*
>  * ExecInitPartitionExtraInfo
>  *  Do the remaining initialization work for the given partition

Yeah, I think the comments on why this is a separate step should be more
extensive.  Probably add something to ExecInitPartitionInfo too.

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

Lack of T_TargetEntry in exprType function

[Konstantin Knizhnik]

Is there any reason for not handling T_TargetEntry node kind in 
exprType() function in nodeFuncs.c?

Is it considered as non-valid expression tag?
But why in this case it is handled in the exprLocation function?

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

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

Hi Amit,

(2018/03/20 15:57), Amit Langote wrote:

On 2018/03/19 20:25, Amit Langote wrote:

That's all I have for now.


Will reply to your previous email.


While testing this patch, I noticed a crash when performing EXPLAIN on
update of a partition tree containing foreign partitions.  Crash occurs in
postgresEndForeignRouting() due to the following Assert failing:

   Assert(fmstate != NULL);

It seems the problem is that ExecCleanupTupleRouting() invokes the
EndForeignRouting() function even if ri_PartitionIsValid is not set.  So I
suppose we need this:

  /*
- * If this is INSERT/UPDATE, allow any FDWs to shut down
+ * If this is INSERT/UPDATE, allow any FDWs to shut down if it has
+ * initialized tuple routing information at all.
   */
  if (node&&
+resultRelInfo->ri_PartitionIsValid&&
  resultRelInfo->ri_FdwRoutine != NULL&&
  resultRelInfo->ri_FdwRoutine->EndForeignRouting != NULL)
  resultRelInfo->ri_FdwRoutine->EndForeignRouting(node->ps.state,


Will look into this.


BTW,patch needs to be rebased because of two commits this morning:
ee49f [1] and ee0a1fc84 [2].


Will do.

Thanks for reviewing the patch!

Best regards,
Etsuro Fujita

Re: ON CONFLICT DO UPDATE for partitioned tables

[Etsuro Fujita]

(2018/03/20 13:30), Amit Langote wrote:

On 2018/03/19 21:59, Etsuro Fujita wrote:

(2018/03/18 13:17), Alvaro Herrera wrote:

Alvaro Herrera wrote:
The only thing that I remain unhappy about this patch is the whole
adjust_and_expand_partition_tlist() thing.  I fear we may be doing
redundant and/or misplaced work.  I'll look into it next week.


I'm still reviewing the patches, but I really agree on that point.  As
Pavan mentioned upthread, the onConflictSet tlist for the root parent,
from which we create a translated onConflictSet tlist for a partition,
would have already been processed by expand_targetlist() to contain all
missing columns as well, so I think we could create the tlist for the
partition by simply re-ordering the expression-converted tlist (ie,
conv_setproj) based on the conversion map for the partition.  The Attached
defines a function for that, which could be called, instead of calling
adjust_and_expand_partition_tlist().  This would allow us to get rid of
planner changes from the patches.  Maybe I'm missing something, though.


Thanks for the patch.  I can confirm your proposed
adjust_onconflictset_tlist() is enough to replace adjust_inherited_tlist()
+ expand_targetlist() combination (aka
adjust_and_expand_partition_tlist()), thus rendering the planner changes
in this patch unnecessary.  I tested it with a partition tree involving
partitions of varying attribute numbers (dropped columns included) and it
seems to work as expected (as also exercised in regression tests) as shown
below.


Thanks for testing!


I have incorporated your patch in the main patch after updating the
comments a bit.  Also, now that ee49f49 is in [1], the transition
table related tests I proposed yesterday pass nicely.  Instead of posting
as a separate patch, I have merged it with the main patch.  So now that
planner refactoring is unnecessary, attached is just one patch.


Here are comments on executor changes in (the latest version of) the patch:

@@ -421,8 +424,18 @@ ExecInsert(ModifyTableState *mtstate,
ItemPointerData conflictTid;
boolspecConflict;
List   *arbiterIndexes;
+   PartitionTupleRouting *proute =
+   
mtstate->mt_partition_tuple_routing;

-   arbiterIndexes = node->arbiterIndexes;
+   /* Use the appropriate list of arbiter indexes. */
+   if (mtstate->mt_partition_tuple_routing != NULL)
+   {
+   Assert(partition_index >= 0 && proute != NULL);
+   arbiterIndexes =
+   
proute->partition_arbiter_indexes[partition_index];
+   }
+   else
+   arbiterIndexes = node->arbiterIndexes;

To handle both cases the same way, I wonder if it would be better to 
have the arbiterindexes list in ResultRelInfo as well, as mentioned by 
Alvaro upthread, or to re-add mt_arbiterindexes as before and set it to 
proute->partition_arbiter_indexes[partition_index] before we get here, 
maybe in ExecPrepareTupleRouting, in the case of tuple routing.


 ExecOnConflictUpdate(ModifyTableState *mtstate,
 ResultRelInfo *resultRelInfo,
+TupleDesc onConflictSetTupdesc,
 ItemPointer conflictTid,
 TupleTableSlot *planSlot,
 TupleTableSlot *excludedSlot,
@@ -1419,6 +1459,7 @@ ExecOnConflictUpdate(ModifyTableState *mtstate,
ExecCheckHeapTupleVisible(estate, , buffer);

/* Store target's existing tuple in the state's dedicated slot */
+   ExecSetSlotDescriptor(mtstate->mt_existing, RelationGetDescr(relation));
ExecStoreTuple(, mtstate->mt_existing, buffer, false);

/*
@@ -1462,6 +1503,7 @@ ExecOnConflictUpdate(ModifyTableState *mtstate,
}

/* Project the new tuple version */
+   ExecSetSlotDescriptor(mtstate->mt_conflproj, onConflictSetTupdesc);
ExecProject(resultRelInfo->ri_onConflictSetProj);

Can we do ExecSetSlotDescriptor for mtstate->mt_existing and 
mtstate->mt_conflproj in ExecPrepareTupleRouting in the case of tuple 
routing?  That would make the API changes to ExecOnConflictUpdate 
unnecessary.


@@ -2368,9 +2419,13 @@ ExecInitModifyTable(ModifyTable *node, EState 
*estate, int eflags)

econtext = mtstate->ps.ps_ExprContext;
relationDesc = resultRelInfo->ri_RelationDesc->rd_att;

-   /* initialize slot for the existing tuple */
-   mtstate->mt_existing =
-   ExecInitExtraTupleSlot(mtstate->ps.state, relationDesc);
+   /*
+* Initialize 

Re: configure's checks for --enable-tap-tests are insufficient

[Dagfinn Ilmari Mannsåker]

Tom Lane  writes:

> Craig Ringer  writes:
>> On 19 March 2018 at 23:56, Tom Lane  wrote:
>>> ... much excavation finds a complaint about Time::HiRes not being installed
>
>> That's absurd. Time::HiRes is a core module, as is Test::More. Sigh.
>
>> Yes, looks like tests are necessary. I'd argue it's "test for broken Perl"
>> but apparently Fedora think they should split the Perl core dist into tiny
>> pieces.

Note that Fedora aren't the only ones to do this: Debian also splits the
Perl core dist into multiple packages.  The crucial difference is that
on Debian installing the 'perl' package gives you the whole lot, while
on Fedora that's just a subset (similar to Debian's 'perl-base'
package); you have to install 'perl-core' to get everything.

> FWIW, the package boundaries are the same on RHEL6, and probably long
> before that.  It's possible that Red Hat have trimmed back which packages
> they consider part of a basic Perl installation.  I don't have any data
> on whether that's moved over time, but it wouldn't surprise me given
> the long-term shifts in Fedora's orientation.

In Fedora 27 (and thus RHEL 8, whenever that will be) they've finally
rearranged the packages so that installing 'perl' gives you the full
Perl core dist, while the minimal subset has been renamed to
'perl-interpreter'.

https://fedoraproject.org/wiki/Changes/perl_Package_to_Install_Core_Modules

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

Re: [HACKERS] Optional message to user when terminating/cancelling backend

[Eren Başak]

Hi,

I reviewed the patch. Here are my notes:

I can confirm that the patches apply and pass the tests as of 03/19/2018 @
11:00am (UTC).

I didn't test whether the docs compile or look good.

I have briefly tested and can confirm that the patch does what is intended.
Here are my comments about the patch:

The patch does not add new tests (maybe isolation tests) for the new
feature. Are there any opportunities to have tests for confirming the new
behavior? At least some regression tests like the following:

SELECT pg_cancel_backend(pg_backend_pid());
ERROR:  57014: canceling statement due to user request

SELECT pg_cancel_backend(pg_backend_pid(), 'message');
ERROR:  57014: canceling statement due to user request: "message"

Not introduced with this patch, but the spacing between the functions is
not consistent in src/backend/storage/ipc/backend_signal.c. There are 2
newlines after some functions (like pg_cancel_backend_msg) and 1 newline
after others (like pg_terminate_backend_msg). It would be nice to fix these
while refactoring.

I also thought about whether the patch should allow the message to be
completely overwritten, instead of appending to the existing one and I
think it is fine. Also, adding the admin message to the HINT or DETAIL part
of the error message would make sense but these are ignored by some clients
so it is fine this way.

Another thing is that, in a similar manner, we could allow changing the
error code which might be useful for extensions. For example, Citus could
use it to cancel remote backends when it detects a distributed deadlock and
changes the error code to something retryable while doing so. For
reference, see the hack that Citus is currently using:
https://github.com/citusdata/citus/blob/81cbb7c223f2eb9cfa8903f1d28869b6f972ded1/src/backend/distributed/shared_library_init.c#L237


+If the optional message parameter is set, the text
+will be appended to the error message returned to the signalled
backend.

I think providing more detail would be useful. For example we can provide
an example about how the error message looks like in its final form or what
will happen if the message is too long.

-pg_signal_backend(int pid, int sig)
+pg_signal_backend(int pid, int sig, char *msg)

The new parameter (msg) is not mentioned in the function header comment.
This applies to pg_signal_backend, pg_cancel_backend_internal,
pg_terminate_backend_internal functions.

+Datum
+pg_cancel_backend(PG_FUNCTION_ARGS)
+{
+ PG_RETURN_BOOL(pg_cancel_backend_internal(PG_GETARG_INT32(0), NULL));
+}
+
+Datum
+pg_cancel_backend_msg(PG_FUNCTION_ARGS)
+{
+ pid_t pid;
+ char*msg = NULL;
+
+ if (PG_ARGISNULL(0))
+ ereport(ERROR,
+ (errmsg("pid cannot be NULL")));
+
+ pid = PG_GETARG_INT32(0);
+
+ if (PG_NARGS() == 2 && !PG_ARGISNULL(1))
+ msg = text_to_cstring(PG_GETARG_TEXT_PP(1));
+
+ PG_RETURN_BOOL(pg_cancel_backend_internal(pid, msg));
+}

The first function seem redundant here because the second one covers all
the cases.

+ memset(slot->message, '\0', sizeof(slot->message));

SetBackendCancelMessage uses memset while BackendCancelShmemInit uses
MemSet. Not a big deal but it would be nice to be consistent and use
postgres macro versions for such calls.

+int
+SetBackendCancelMessage(pid_t backend, char *message)
+{
+ BackendCancelShmemStruct *slot;

The variable "slot" is declared outside of the foor loop but only used in
the inside, therefore it would be nicer to declare it inside the loop.

+ BackendCancelInit(MyBackendId);

Is the "normal multiuser case" the best place to initialize cancellation?
For example, can't we cancel background workers? If this is the right
place, maybe we should justify why this is the best place to initialize
backend cancellation memory part with a comment.

+ char   *buffer = palloc0(MAX_CANCEL_MSG);

Why not use a static array like char[MAX_CANCEL_MSG], instead of pallocing?

+/*
+ * Return the configured cancellation message and its length. If the
returned
+ * length is greater than the size of the passed buffer, truncation has
been
+ * performed. The message is cleared on reading.
+ */
+int
+GetCancelMessage(char **buffer, size_t buf_len)
+{
+ volatile BackendCancelShmemStruct *slot = MyCancelSlot;
+ int msg_length = 0;
+
+ if (slot != NULL && slot->len > 0)
+ {
+ SpinLockAcquire(>mutex);
+ strlcpy(*buffer, (const char *) slot->message, buf_len);
+ msg_length = slot->len;
+ slot->len = 0;
+ slot->message[0] = '\0';
+ SpinLockRelease(>mutex);
+ }
+
+ return msg_length;
+}

We never change what the buffer points to, therefore is it necessary to
declare `buffer` as char** instead of char*?

+extern int SetBackendCancelMessage(pid_t backend, char *message);

Maybe rename backend to something like backend_pid.

+/*
+ * Return the configured cancellation message and its length. If the
returned
+ * length is greater than the size of the passed buffer, truncation has
been
+ * performed. The message is cleared on reading.
+ */
+int
+GetCancelMessage(char **buffer, 

Define variable only in the scope that needs it

[Antonin Houska]

I suppose the current definition in the higher scope is not intentional. This
is what I suggest:

diff --git a/src/backend/optimizer/plan/planner.c 
b/src/backend/optimizer/plan/planner.c
new file mode 100644
index 9c4a1ba..ae35e1e
*** a/src/backend/optimizer/plan/planner.c
--- b/src/backend/optimizer/plan/planner.c
*** create_ordinary_grouping_paths(PlannerIn
*** 3837,3843 
  {
Query  *parse = root->parse;
Path   *cheapest_path = input_rel->cheapest_total_path;
-   AggClauseCosts agg_partial_costs;   /* parallel only */
AggClauseCosts agg_final_costs; /* parallel only */
double  dNumGroups;
boolcan_hash;
--- 3837,3842 
*** create_ordinary_grouping_paths(PlannerIn
*** 3904,3909 
--- 3903,3909 
if (try_parallel_aggregation)
{
PathTarget *partial_grouping_target;
+   AggClauseCosts agg_partial_costs;   /* parallel only */
  
/*
 * Build target list for partial aggregate paths.  These paths 
cannot


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

Re: XID-assigned idle transactions affect vacuum's job.

[Amit Kapila]

On Tue, Mar 20, 2018 at 12:22 PM, Masahiko Sawada  wrote:
> Hi,
>
> Long transactions often annoy users because if a long transaction
> exists on a database vacuum cannot reclaim efficiently. There are
> several reason why they exist on a database but it's a common case
> where users or applications forget to commit/rollback transactions.
> That is, transaction is not executing SQL and its state is 'idle in
> transaction' on pg_stat_activity. In this case, such transactions
> don't affect vacuum's job either if they aren't assigned transaction
> id or if they don't have a snapshot. However if they have xid it will
> affect vacuum's job even if they don't have a snapshot.
>
> I think that to decide which deleted tuples must be preserved we don't
> need to care about backend PGXACT.xid but must care about PGXACT.xmin.
> But current GetOldestXmin considers both of them. I guess one reason
> why GetOldestXmin does so is that it's also used to determine where to
> truncate pg_subtrans. Is there anything else reason?
>

I think the main reason is that while computing snapshots, we also
rely on PGXACT.xid.  Basically, it can be present in some other
snapshots xmin.  Now, if you ignore it in vacuum (GetOldestXmin), then
it is quite possible that the xid we have ignored will be part of some
other snapshot's xmin which I think in turn can lead to wrong results.

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

Re: JIT compiling with LLVM v12.2

[Andres Freund]

Hi,

I've pushed a revised and rebased version of my JIT patchset.
The git tree is at
  https://git.postgresql.org/git/users/andresfreund/postgres.git
in the jit branch
  
https://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=shortlog;h=refs/heads/jit

There's a lot of tiny changes in here:
- doc proofreading, addition of --with-llvm docs
- comments
- pgindent (jit files, not whole tree)
- syncing of order between compiled and interpreted expressions in case
  statement
- line-by-line review of expression compliation
- fix of a small memory leak (missing pfree of the JIT context struct
  itself)
- slight simplification of JIT resowner integration (no need to
  re-associate with parent)

My current plan is to push the first few commits relatively soon, give
the BF a few cycles to shake out. Set up a few BF animals with each
supported LLVM version. Then continue mergin.

Greetings,

Andres Freund

Re: [HACKERS] [PATCH] Generic type subscripting

[Oleksandr Shulgin]

On Tue, Mar 6, 2018 at 6:21 PM, Dmitry Dolgov <9erthali...@gmail.com> wrote:

>
> One more small update after fd1a421fe6 in attachments.
>

Before looking at the code I have a few comments about documentation:

in json.sgml:

+-- Extract value by key
+SELECT ('{"a": 1}'::jsonb)['a'];

What is the result of running this query?  What is the resulting data type?

+-- Extract nested value by key path
+SELECT ('{"a": {"b": {"c": 1}}}'::jsonb)['a']['b']['c'];
+
+-- Extract element by index
+SELECT ('[1, "2", null]'::jsonb)['1'];

What is the result here?  Why subscript is a string and not a number?  Are
subscription indexes 0- or 1-based?

+-- Update value by key
+UPDATE table_name set jsonb_field['key'] = 1;
+
+-- Select records using where clause with subscripting
+SELECT * from table_name where jsonb_field['key'] = '"value"';

Please capitalize: SET, FROM, WHERE.

Use of double quotes around "value" requires some explanation, I think.
Should the user expect that a suitable index is used by the query planner
for this query?

In other words, I would like to see this part of documentation to be
extended beyond just showcasing the syntax.

Regards,
-- 
Oleksandr "Alex" Shulgin | Database Engineer | Zalando SE | Tel: +49 176
127-59-707

Re: PostgreSQL 10: Segmentation fault when using GROUPING SETS with all unsortable columns

[Andrew Gierth]

> "Andrew" == Andrew Gierth  writes:

 Huong> Not yet fully understand the related commit, but I think it is
 Huong> fine to put ERRCODE_FEATURE_NOT_SUPPORTED error from
 Huong> preprocess_grouping_sets when all columns in GROUPING SETS are
 Huong> unsortable. Is that right?

 Andrew> No, that's definitely wrong. The intent is to be able to
 Andrew> generate a hashed path in this case, it's just the logic that
 Andrew> tries to prefer sorting to hashing when the input arrives
 Andrew> already sorted is doing the wrong thing for unsortable data.

Attached is what I think is the correct fix, which I'll commit shortly.

-- 
Andrew (irc:RhodiumToad)

diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 9c4a1baf5f..7fabe017c2 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -4017,7 +4017,28 @@ consider_groupingsets_paths(PlannerInfo *root,
 
 		Assert(can_hash);
 
-		if (pathkeys_contained_in(root->group_pathkeys, path->pathkeys))
+		/*
+		 * If the input is coincidentally sorted usefully (which can happen
+		 * even if is_sorted is false, since that only means that our caller
+		 * has set up the sorting for us), then save some hashtable space by
+		 * making use of that. But we need to watch out for degenerate cases:
+		 *
+		 * 1) If there are any empty grouping sets, then group_pathkeys might
+		 * be NIL if all non-empty grouping sets are unsortable. In this case,
+		 * there will be a rollup containing only empty groups, and the
+		 * pathkeys_contained_in test is vacuously true; this is ok.
+		 *
+		 * XXX: the above relies on the fact that group_pathkeys is generated
+		 * from the first rollup. If we add the ability to consider multiple
+		 * sort orders for grouping input, this assumption might fail.
+		 *
+		 * 2) If there are no empty sets and only unsortable sets, then the
+		 * rollups list will be empty (and thus l_start == NULL), and
+		 * group_pathkeys will be NIL; we must ensure that the vacuously-true
+		 * pathkeys_contain_in test doesn't cause us to crash.
+		 */
+		if (l_start != NULL &&
+			pathkeys_contained_in(root->group_pathkeys, path->pathkeys))
 		{
 			unhashed_rollup = lfirst_node(RollupData, l_start);
 			exclude_groups = unhashed_rollup->numGroups;
diff --git a/src/test/regress/expected/groupingsets.out b/src/test/regress/expected/groupingsets.out
index d21a494a9d..c7deec2ff4 100644
--- a/src/test/regress/expected/groupingsets.out
+++ b/src/test/regress/expected/groupingsets.out
@@ -1018,6 +1018,18 @@ explain (costs off)
  ->  Values Scan on "*VALUES*"
 (9 rows)
 
+-- unsortable cases
+select unsortable_col, count(*)
+  from gstest4 group by grouping sets ((unsortable_col),(unsortable_col))
+  order by unsortable_col::text;
+ unsortable_col | count 
++---
+  1 | 4
+  1 | 4
+  2 | 4
+  2 | 4
+(4 rows)
+
 -- mixed hashable/sortable cases
 select unhashable_col, unsortable_col,
grouping(unhashable_col, unsortable_col),
diff --git a/src/test/regress/sql/groupingsets.sql b/src/test/regress/sql/groupingsets.sql
index eb68028603..c32d23b8d7 100644
--- a/src/test/regress/sql/groupingsets.sql
+++ b/src/test/regress/sql/groupingsets.sql
@@ -292,6 +292,11 @@ explain (costs off)
   select a, b, grouping(a,b), array_agg(v order by v)
 from gstest1 group by cube(a,b);
 
+-- unsortable cases
+select unsortable_col, count(*)
+  from gstest4 group by grouping sets ((unsortable_col),(unsortable_col))
+  order by unsortable_col::text;
+
 -- mixed hashable/sortable cases
 select unhashable_col, unsortable_col,
grouping(unhashable_col, unsortable_col),

Re: Online enabling of checksums

[Magnus Hagander]

On Tue, Mar 20, 2018 at 10:29 AM, Andrey Borodin 
wrote:

> Hi!
>
> > 19 марта 2018 г., в 11:27, Heikki Linnakangas 
> написал(а):
> >
> > Is there no way to stop the checksum helper once it's started? That
> seems rather user-unfriendly. I can imagine it being a pretty common
> mistake to call pg_enable_data_checksums() on a 10 TB cluster, only to
> realize that you forgot to set the cost limit, and that it's hurting
> queries too much. At that point, you want to abort.
> I've tried to pg_cancel_backend() and it worked.
> But only if I cancel "checksum helper launcher" and then "checksum helper
> worker". If I cancel helper first - it spawns new.
>
> Magnus, Daniel, is it safe to cancel worker or launcher?
>

It should be perfectly safe yes. However, it's not very user friendly,
because you have to go around and cancel both (meaning you actually have to
cancel them in the right order, or a new one will be launched).

Daniel is working on a proper way to stop things.



>
> BTW, I have some questions on pg_verify_chechsums.
> It does not check catalog version. It it true that it will work for any?
>

Yes. The actual page format for checksums has not changed since checksums
were introduced. I have successfully used it to validate checksums on v10
clusters for example.



> Also, pg_verify_chechsums will stop on short reads. But we do not stop on
> wrong checksum, may be we should not stop on short reads either?
>


These are very different scenarios though -- it's explicitly intended to
validate checksums, and short reads is a different issue. I prefer the way
it does it now, but I am not strongly locked into that position and can be
convinced otherwise :)

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: INOUT parameters in procedures

[Rushabh Lathia]

On Tue, Mar 20, 2018 at 6:38 AM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 3/19/18 03:25, Rushabh Lathia wrote:
> > For the FUNCTION when we have single OUT/INOUT parameter
> > the return type for that function will be set to the type of OUT
> parameter.
> > But in case of PROCEDURE, it's always RECORDOID, why this inconsistency?
>
> For procedures, this is just an implementation detail.  The CALL command
> returns a row in any case, so if we set the return type to a scalar
> type, we'd have to add special code to reassemble a row anyway.  For
> functions, the inconsistency is (arguably) worth it, because it affects
> how functions can be written and called, but for procedures, there would
> be no point.
>
>
This feel like inconsistency with the existing system object FUNCTION.
It would be nice to be consistent with the FUNCTION - which set the
prorettype as the type of single IN/OUT in case of single argument.

If CALL command returns a row in any case, then I think adding logic
to build row while building the output for CALL statement make more sense.


> > Above test throws an error saying calling procedures with output
> > arguments are not supported in SQL functions.  Whereas similar test
> > do work with SQL functions:
>
> This was discussed earlier in the thread.
>
> The behavior of output parameters in functions was, AFAICT, invented by
> us.  But for procedures, the SQL standard specifies it, so there might
> be some differences.
>
>
Sorry, but I am still unable to understand the difference.
In case of PROCEDURE, it's calling the PROCEDURE with out parameter.
So if that we call the same PROCEURE in the psql prompt:

postgres@101361=#CALL ptest4a(null, null);
 a | b
---+---
 1 | 2
(1 row)

and same is the case if we call the FUNCTION in the psql prompt:

postgres@101361=#SELECT * from ftest4b(null, null);
 b | a
---+---
 1 | 2
(1 row)

So if I understand correctly, in the testcase where it's calling the CALL
within SQL procedure - has to throw similar output. Isn't it?


> ERROR:  calling procedures with output arguments is not supported in SQL
> > functions
> > CONTEXT:  SQL function "ptest4b"
> >
> > Here error message says that calling procedures with output arguments is
> not
> > supported in SQL functions.  Whereas here it's getting called from the
> SQL
> > procedure.  So error message needs to be changed.
>
> Well, I don't think we are going to change every single error message
> from "function" to a separate function and procedure variant.
>
>
I think we should, otherwise it pass the wrong message to the user. Like
here it says "calling procedures with output arguments is not supported in
SQL functions"
but actually test is calling the procedures from procedure.  I think now
that
we have a way to ideintify FUNCTION/PROCEDURE (prokind) it's good
to give proper error message.

Recently commit 2c6f37ed62114bd5a092c20fe721bd11b3bcb91e and
8b9e9644dc6a9bd4b7a97950e6212f63880cf18b replace AclObjectKind and
GrantObjectType with ObjectType and with that we now getting proper
object type for the acl error message. In case of PROCEDURE
and FUNCTIONS also error message should send clear message.


Regards,
Rushabh Lathia
www.EnterpriseDB.com

Re: Online enabling of checksums

[Andrey Borodin]

Hi!

> 19 марта 2018 г., в 11:27, Heikki Linnakangas  написал(а):
> 
> Is there no way to stop the checksum helper once it's started? That seems 
> rather user-unfriendly. I can imagine it being a pretty common mistake to 
> call pg_enable_data_checksums() on a 10 TB cluster, only to realize that you 
> forgot to set the cost limit, and that it's hurting queries too much. At that 
> point, you want to abort.
I've tried to pg_cancel_backend() and it worked.
But only if I cancel "checksum helper launcher" and then "checksum helper 
worker". If I cancel helper first - it spawns new.

Magnus, Daniel, is it safe to cancel worker or launcher?

BTW, I have some questions on pg_verify_chechsums.
It does not check catalog version. It it true that it will work for any?
Also, pg_verify_chechsums will stop on short reads. But we do not stop on wrong 
checksum, may be we should not stop on short reads either?

I agree with all of Heikki's comments.

Besides these I have no other questions, patch looks good.

Best regards, Andrey Borodin.

Re: IndexJoin memory problem using spgist and boxes

[Anton Dignös]

On Tue, Mar 20, 2018 at 5:01 AM, Tom Lane  wrote:
> Alexander Kuzmenkov  writes:
>> The updated version looks good to me.
>
> LGTM too.  Pushed with some trivial cosmetic adjustments.
>
> regards, tom lane

Thank you both!

Best regards,
Anton

Re: [PoC PATCH] Parallel dump to /dev/null

[Christoph Berg]

Re: Alvaro Herrera 2018-03-05 <20180305165609.kq5y7uzy64o45vsg@alvherre.pgsql>
> The reason I noticed is I wondered about the amount of open() calls
> (plus zlib function calls) we would save by keeping an FD open to
> /dev/null, rather than opening it over and over for each object -- ie.,
> maybe not touch setFilePath() at all, if possible.  That looks perhaps
> too invasive, so maybe not.  But do audit other callsites that may open
> files.

In normal operation, the files are also opened individually, so
special-casing /dev/null seems overly specific to me (and I'd be very
surprised if it made any difference.)

For the feature to be useful in practise, it needs documentation.
People won't expect -Fd -f /dev/null to work because /dev/null is not
a directory.

Otherwise, +1 from me.
Christoph

Re: PostgreSQL 10: Segmentation fault when using GROUPING SETS with all unsortable columns

[Andrew Gierth]

> "Huong" == Huong Dangminh  writes:

 Huong> Hi,

 Huong> I have found a case which could get segmentation fault when
 Huong> using GROUPING SETS. It occurs when columns in GROUPING SETS are
 Huong> all unsortable but hashable.

Yes, mea culpa. will fix.

BTW, you should have reported this as a bug via the pgsql-bugs list or
the bug submission form.

 Huong> Attached grouping_sets_segv.zip include module to reproduce this
 Huong> problem.

A much simpler way to reproduce it is to use one of the builtin types
that isn't sortable (I tend to use xid for testing).

 Huong> Not yet fully understand the related commit, but I think it is
 Huong> fine to put ERRCODE_FEATURE_NOT_SUPPORTED error from
 Huong> preprocess_grouping_sets when all columns in GROUPING SETS are
 Huong> unsortable. Is that right?

No, that's definitely wrong. The intent is to be able to generate a
hashed path in this case, it's just the logic that tries to prefer
sorting to hashing when the input arrives already sorted is doing the
wrong thing for unsortable data.

-- 
Andrew (irc:RhodiumToad)

possibly to automatic detection of bad variable types in plans?

[Pavel Stehule]

Hi

I have a talk with Alex Stanev about known issue that block indexes - bad
param types

postgres=# do $$
declare _id numeric = 22;
begin
  perform * from bigtable where a = _id;
end;
$$;
DO
Time: 108,775 ms
postgres=# do $$
declare _id numeric = 22;
begin
  perform * from bigtable where a = _id;
end;
$$;
DO
Time: 106,111 ms
postgres=# do $$
declare _id int = 22;
begin
  perform * from bigtable where a = _id;
end;
$$;
DO
Time: 1,522 ms
postgres=# do $$
declare _id numeric = 22;
begin
  perform * from bigtable where a = _id;
end;
$$;
DO
Time: 108,287 ms

https://github.com/okbob/plpgsql_check/issues/32

Is possible to write a extension, that raise warning so index is not
possible due bad parameter type, or so cast will be performed on every row?

It can be started like autoexplain.

Regards

Pavel

Re: PostgreSQL opens all the indexes of a relation for every Query during Planning Time?

[Meenatchi Sandanam]

Please refer this https://www.postgresql.org/message-id/CAJJu9dMg3662E-
wr25zyajwvdg7cz56r-8bw4ejft9i42oo...@mail.gmail.com

On Fri, Mar 16, 2018 at 10:48 PM, Jason Petersen 
wrote:

> On Mar 15, 2018, at 4:18 AM, Meenatchi Sandanam 
> wrote:
>
> On checking PostgreSQL code, we see it opens all indexes of a relation for
> every query planning time as in the attachment. If that is the case, it
> might lead to Performance degradation for relations with many indexes. Is
> there any other better way to handle this like Scanning/Opening only the
> indexes related to columns specified in the query?
>
>
> I’d think you’d need a pretty clear profiling run that points to this as a
> hot spot before deciding to optimize it. Do you have a query where this is
> the case?
>
> --
> *Jason Petersen*
> Software Engineer | Citus Data
> 303.736.9255
> ja...@citusdata.com
>
>

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Amit Langote]

On 2018/03/19 20:25, Amit Langote wrote:
> That's all I have for now.

While testing this patch, I noticed a crash when performing EXPLAIN on
update of a partition tree containing foreign partitions.  Crash occurs in
postgresEndForeignRouting() due to the following Assert failing:

  Assert(fmstate != NULL);

It seems the problem is that ExecCleanupTupleRouting() invokes the
EndForeignRouting() function even if ri_PartitionIsValid is not set.  So I
suppose we need this:

 /*
- * If this is INSERT/UPDATE, allow any FDWs to shut down
+ * If this is INSERT/UPDATE, allow any FDWs to shut down if it has
+ * initialized tuple routing information at all.
  */
 if (node &&
+resultRelInfo->ri_PartitionIsValid &&
 resultRelInfo->ri_FdwRoutine != NULL &&
 resultRelInfo->ri_FdwRoutine->EndForeignRouting != NULL)
 resultRelInfo->ri_FdwRoutine->EndForeignRouting(node->ps.state,

BTW,patch needs to be rebased because of two commits this morning:
ee49f [1] and ee0a1fc84 [2].

Thanks,
Amit

[1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=ee49f

[2] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=ee0a1fc84

XID-assigned idle transactions affect vacuum's job.

[Masahiko Sawada]

Hi,

Long transactions often annoy users because if a long transaction
exists on a database vacuum cannot reclaim efficiently. There are
several reason why they exist on a database but it's a common case
where users or applications forget to commit/rollback transactions.
That is, transaction is not executing SQL and its state is 'idle in
transaction' on pg_stat_activity. In this case, such transactions
don't affect vacuum's job either if they aren't assigned transaction
id or if they don't have a snapshot. However if they have xid it will
affect vacuum's job even if they don't have a snapshot.

I think that to decide which deleted tuples must be preserved we don't
need to care about backend PGXACT.xid but must care about PGXACT.xmin.
But current GetOldestXmin considers both of them. I guess one reason
why GetOldestXmin does so is that it's also used to determine where to
truncate pg_subtrans. Is there anything else reason? If nothing, I'd
like to change GetOldestXmin so that it sees only PGXACT.xmin for
vacuum purposes. Once we addressed this issue it'll helpful especially
for user who uses read committed transaction isolation level.

Regards,

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

Re: Problem while setting the fpw with SIGHUP

[Dilip Kumar]

On Tue, Mar 20, 2018 at 11:26 AM, Michael Paquier 
wrote:

> On Tue, Mar 20, 2018 at 10:43:55AM +0530, Dilip Kumar wrote:
> > I think like WALWriterProcess, we need to call InitXLogInsert for the
> > CheckpointerProcess as well as for the BgWriterProcess
> > because earlier they were calling InitXLogInsert while check
> > RecoveryInProgress before inserting the WAL.
>
> /* don't set signals, bgwriter has its own agenda */
> +   InitXLOGAccess();
> +   InitXLogInsert()
>
> This is wrong, as the checkpointer is started as well on standbys, and
> that InitXLOGAccess initializes things for WAL generation like
> ThisTimeLineID.  So you should just call InitXLogInsert(), and a comment
> would be welcome for both the bgwriter and the checkpointer.
>

Yeah, you are right.  Fixed.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


xlog-insert-critical-v3.patch
Description: Binary data