Re: Minimal logical decoding on standbys

[Amit Khandekar]

On Thu, 30 May 2019 at 20:13, Andres Freund  wrote:
>
> Hi,
>
> On 2019-05-30 19:46:26 +0530, Amit Khandekar wrote:
> > @@ -1042,7 +1042,8 @@ ReplicationSlotReserveWal(void)
> >  if (!RecoveryInProgress() && SlotIsLogical(slot))
> >  {
> > 
> >  }
> >  else
> >  {
> > -   restart_lsn = GetRedoRecPtr();
> > +   restart_lsn = SlotIsLogical(slot) ?
> > +GetXLogReplayRecPtr() : 
> > GetRedoRecPtr();
> >
> > But then when I do pg_create_logical_replication_slot(), it hangs in
> > DecodingContextFindStartpoint(), waiting to find new records
> > (XLogReadRecord).
>
> But just till the primary has logged the necessary WAL records? If you
> just do CHECKPOINT; or such on the primary, it should succeed quickly?

Yes, it waits until there is a commit record, or (just tried) until a
checkpoint command.

>
> Greetings,
>
> Andres Freund



-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company

Re: New committer: David Rowley

[Alexander Korotkov]

On Thu, May 30, 2019 at 6:39 PM Magnus Hagander  wrote:
> For those of you that have not read the minutes from the developer meeting 
> ahead of pgcon (can be found at 
> https://wiki.postgresql.org/wiki/PgCon_2019_Developer_Meeting), we'd like to 
> announce here as well that David Rowley has joined the ranks of PostgreSQL 
> committers.
>
> Congratulations to David, may the buildfarm be gentle to him, and his first 
> revert far away!

+1

Congratulations to David!  Very much deserved!

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

Re: Fix inconsistencies for v12

[Amit Langote]

On 2019/05/30 18:51, Amit Kapila wrote:
> On Wed, May 29, 2019 at 6:12 AM Amit Langote wrote:
>> On 2019/05/28 20:26, Amit Kapila wrote:
>>> Can we ensure some way that only FDW's rely on it not any other part
>>> of the code?
>>
>> Hmm, I can't think of any way of doing than other than manual inspection.
>> We are sure that no piece of core code relies on it in the ExecInitNode()
>> code path.  Apparently FDWs may, as we've found out here.  Now that I've
>> looked around, maybe other loadable modules may too, by way of (only?)
>> Custom nodes.  I don't see any other way to hook into ExecInitNode(), so
>> maybe that's it.
>>
>> So, maybe reword a bit as:
>>
>> diff --git a/src/backend/executor/nodeModifyTable.c
>> b/src/backend/executor/nodeModifyTable.c
>> index a3c0e91543..95545c9472 100644
>> --- a/src/backend/executor/nodeModifyTable.c
>> +++ b/src/backend/executor/nodeModifyTable.c
>> @@ -2316,7 +2316,7 @@ ExecInitModifyTable(ModifyTable *node, EState
>> *estate, int eflags)
>>   * verify that the proposed target relations are valid and open their
>>   * indexes for insertion of new index entries.  Note we *must* set
>>   * estate->es_result_relation_info correctly while we initialize each
>> - * sub-plan; ExecContextForcesOids depends on that!
>> + * sub-plan; external modules such as FDWs may depend on that.
>>
> 
> I think it will be better to include postgres_fdw in the comment in
> some way so that if someone wants a concrete example, there is
> something to refer to.

Maybe a good idea, but this will be the first time to mention postgres_fdw
in the core source code.  If you think that's OK, how about the attached?

Thanks,
Amit
diff --git a/src/backend/executor/nodeModifyTable.c 
b/src/backend/executor/nodeModifyTable.c
index a3c0e91543..da6da06115 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -2316,7 +2316,9 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, 
int eflags)
 * verify that the proposed target relations are valid and open their
 * indexes for insertion of new index entries.  Note we *must* set
 * estate->es_result_relation_info correctly while we initialize each
-* sub-plan; ExecContextForcesOids depends on that!
+* sub-plan; external modules such as FDWs may depend on that (see
+* contrib/postgres_fdw/postgres_fdw.c: postgresBeginDirectModify()
+* as one example.)
 */
saved_resultRelInfo = estate->es_result_relation_info;
 

Re: New committer: David Rowley

[Amit Langote]

On 2019/05/31 0:39, Magnus Hagander wrote:
> Hi!
> 
> For those of you that have not read the minutes from the developer meeting
> ahead of pgcon (can be found at
> https://wiki.postgresql.org/wiki/PgCon_2019_Developer_Meeting), we'd like
> to announce here as well that David Rowley has joined the ranks of
> PostgreSQL committers.
> 
> Congratulations to David, may the buildfarm be gentle to him, and his first
> revert far away!

Very well deserved, congratulations!

Thanks,
Amit

Re: How to know referenced sub-fields of a composite type?

[Kohei KaiGai]

2019/05/30 16:33、Amit Langote のメール:

>> On 2019/05/29 15:50, Kohei KaiGai wrote:
>> 2019年5月29日(水) 13:26 Amit Langote :
 It means we can skip to load the sub-fields unreferenced, if
 query-planner can handle
 referenced and unreferenced sub-fields correctly.
 On the other hands, it looks to me RelOptInfo or other optimizer
 related structure don't have
 this kind of information. RelOptInfo->attr_needed tells extension
 which attributes are referenced
 by other relation, however, its granularity is not sufficient for 
 sub-fields.
>>> 
>>> Isn't that true for some other cases as well, like when a query accesses
>>> only some sub-fields of a json(b) column?  In that case too, planner
>>> itself can't optimize away access to other sub-fields.  What it can do
>>> though is match a suitable index to the operator used to access the
>>> individual sub-fields, so that the index (if one is matched and chosen)
>>> can optimize away accessing unnecessary sub-fields.  IOW, it seems to me
>>> that the optimizer leaves it up to the indexes (and plan nodes) to further
>>> optimize access to within a field.  How is this case any different?
>> 
>> I think it is a little bit different scenario.
>> Even if an index on sub-fields can indicate the tuples to be fetched,
>> the fetched tuple contains all the sub-fields because heaptuple is
>> row-oriented data.
>> 
>> For example, if WHERE-clause checks a sub-field: "x" then aggregate
>> function references other sub-field "y", Scan/Join node has to return
>> a tuple that contains both "x" and "y". IndexScan also pops up a tuple
>> with a full composite type, so here is no problem if we cannot know
>> which sub-fields are referenced in the later stage.
>> Maybe, if IndexOnlyScan supports to return a partial composite type,
>> it needs similar infrastructure that can be used for a better composite
>> type support on columnar storage.
> 
> Ah indeed.  I think I had misunderstood your intent.  Indexes have to do
> with optimizing the "filtering" of complex/nested type (json, Arrow
> Struct, etc.) values, where unnecessary sub-fields need not be read before
> filtering, whereas you're interested in optimizing "projections" of
> complex types, where sub-fields that are not used anywhere in the query
> need not be read from the stored values.
> 
 Probably, all we can do right now is walk-on the RelOptInfo list to
 lookup FieldSelect node
 to see the referenced sub-fields. Do we have a good idea instead of
 this expensive way?
 # Right now, PG-Strom loads all the sub-fields of Struct column from
 arrow_fdw foreign-table
 # regardless of referenced / unreferenced sub-fields. Just a second best.
>>> 
>>> I'm missing something, but if PG-Strom/arrow_fdw does look at the
>>> FieldSelect nodes to see which sub-fields are referenced, why doesn't it
>>> generate a plan that will only access those sub-fields or why can't it?
>>> 
>> Likely, it is not a technical problem but not a smart implementation.
>> If I missed some existing infrastructure we can apply, it may be more 
>> suitable
>> than query/expression tree walking.
> 
> There is no infrastructure for this as far as I know.  Maybe, some will be
> built in the future now that storage format is pluggable.

If we design a common infrastructure for both of built-in and extension 
features, it makes sense for the kinds of storage system.
IndexOnlyScan is one of the built-in feature that is beneficial by the 
information of projection. Currently, we always don’t choose IndexOnlyScan if 
index is on sub-field of composite.

 Best regards,

Re: incorrect xlog.c coverage report

[Michael Paquier]

On Wed, May 29, 2019 at 12:09:08PM -0400, Alvaro Herrera wrote:
> Are there objections to doing that now on the master branch?

Adding the flush call just on HEAD is fine for me.  Not sure that
there is an actual reason to back-patch that.
--
Michael


signature.asc
Description: PGP signature

Re: cpluspluscheck vs vpath

[Alvaro Herrera]

On 2019-May-30, Andres Freund wrote:

> On 2019-05-30 15:02:44 -0700, Andres Freund wrote:
>
> > Seems we could round the edges a good bit further than what's done in
> > the attached (argument checking, for example, but also using the C++
> > compiler from configure). But I think this would already be an
> > improvement?

+1  I've stumbled upon this too.

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

Re: cpluspluscheck vs vpath

[Andres Freund]

Hi,

On 2019-05-30 15:02:44 -0700, Andres Freund wrote:
> right now cpluspluscheck doesn't work with vpath builds. That's pretty
> annoying, because it does require cloning the git tree into a separate
> directory + doing configure there just to run cpluspluscheck.
> 
> Attached is a small patch allowing cpluspluscheck to run from different
> directories. I needs the src and build directories for that,
> unsurprisingly.
> 
> As that makes it more complicated to invoke, I added a makefile target
> (in the top level) for it.
> 
> Seems we could round the edges a good bit further than what's done in
> the attached (argument checking, for example, but also using the C++
> compiler from configure). But I think this would already be an
> improvement?

Ugh, sent the previous email too early.

Greetings,

Andres Freund
>From 647934bd150684ec97a51005b953ad6d8207ded5 Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Thu, 30 May 2019 14:59:47 -0700
Subject: [PATCH] integrate cpluspluscheck a bit more

---
 GNUmakefile.in |  3 +++
 src/tools/pginclude/cpluspluscheck | 13 +++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/GNUmakefile.in b/GNUmakefile.in
index f4e31a7c5f1..6242ece2492 100644
--- a/GNUmakefile.in
+++ b/GNUmakefile.in
@@ -128,4 +128,7 @@ distcheck: dist
 	rm -rf $(distdir) $(dummy)
 	@echo "Distribution integrity checks out."
 
+cpluspluscheck: submake-generated-headers
+	$(top_srcdir)/src/tools/pginclude/cpluspluscheck $(top_srcdir) $(abs_top_builddir)
+
 .PHONY: dist distdir distcheck docs install-docs world check-world install-world installcheck-world
diff --git a/src/tools/pginclude/cpluspluscheck b/src/tools/pginclude/cpluspluscheck
index 3a771c48b62..02f5ca565fa 100755
--- a/src/tools/pginclude/cpluspluscheck
+++ b/src/tools/pginclude/cpluspluscheck
@@ -1,9 +1,15 @@
 #!/bin/sh
 
 # Check all exported PostgreSQL include files for C++ compatibility.
-# Run this from the top-level source directory after performing a build.
+#
+# Argument 1 is the source directory, argument 2 the build directory
+# (they might be the same).
+#
 # No output if everything is OK, else compiler errors.
 
+srcdir="$1"
+builddir="$2"
+
 me=`basename $0`
 
 tmp=`mktemp -d /tmp/$me.XX`
@@ -22,6 +28,7 @@ trap 'rm -rf $tmp' 0 1 2 3 15
 # which itself contains C++ code and so won't compile with a C++
 # compiler under extern "C" linkage.
 
+cd "$srcdir"
 for f in `find src/include src/interfaces/libpq/libpq-fe.h src/interfaces/libpq/libpq-events.h -name '*.h' -print | \
 grep -v -e ^src/include/port/ \
 	-e ^src/include/rusagestub.h -e ^src/include/regex/regerrs.h \
@@ -36,5 +43,7 @@ do
 	echo '};'
 	} >$tmp/test.cpp
 
-	${CXX:-g++} -I . -I src/interfaces/libpq -I src/include -fsyntax-only -Wall -c $tmp/test.cpp
+	${CXX:-g++} -I $srcdir -I $srcdir/src/interfaces/libpq -I $srcdir/src/include \
+		-I $builddir -I $builddir/src/interfaces/libpq -I $builddir/src/include \
+		-fsyntax-only -Wall -c $tmp/test.cpp
 done
-- 
2.21.0.dirty

Re: Ought to use heap_multi_insert() for pg_attribute/depend insertions?

[Andres Freund]

Hi,

On 2019-05-22 10:25:14 +0200, Daniel Gustafsson wrote:
> It passes make check and some light profiling around regress suites indicates
> that it does improve a bit by reducing the somewhat costly calls.

Just for the record, here is the profile I did:

perf record --call-graph lbr make -s check-world -Otarget -j16 -s
perf report

Greetings,

Andres Freund

cpluspluscheck vs vpath

[Andres Freund]

Hi,

right now cpluspluscheck doesn't work with vpath builds. That's pretty
annoying, because it does require cloning the git tree into a separate
directory + doing configure there just to run cpluspluscheck.

Attached is a small patch allowing cpluspluscheck to run from different
directories. I needs the src and build directories for that,
unsurprisingly.

As that makes it more complicated to invoke, I added a makefile target
(in the top level) for it.

Seems we could round the edges a good bit further than what's done in
the attached (argument checking, for example). But I think this would
already be an improvement?

Greetings,

Andres Freund

Re: tableam.h fails cpluspluscheck

[Andres Freund]

Hi,

On 2019-05-30 14:01:00 -0400, Tom Lane wrote:
> Using Apple's clang as c++ compiler:
> 
> In file included from /tmp/cpluspluscheck.KejiIw/test.cpp:3:
> ./src/include/access/tableam.h:144:16: error: typedef redefinition with 
> different types ('void (*)(Relation, HeapTuple, Datum *, bool *, bool, void 
> *)' (aka 'void (*)(RelationData *, HeapTupleData *, unsigned long *, bool *, 
> bool, void *)') vs 'IndexBuildCallback')
> typedef void (*IndexBuildCallback) (Relation index,
>^
> ./src/include/access/tableam.h:36:8: note: previous definition is here
> struct IndexBuildCallback;
>^
> 
> (there are some cascading errors, but this is the important one)
> 
> Kinda looks like you can't get away with using "struct" on a forward
> declaration of something that is not actually a struct type.

Ugh. Odd that only C++ compilers complain. I just removed the typedef,
it's not needed anymore (it used to be neccessary before moving
IndexBuildCallback's definition to tableam.h - but was wrong then too,
just cpluspluscheck didn't notice).

Pushed the obvious fix.

Greetings,

Andres Freund

Re: New committer: David Rowley

[Michael Paquier]

On Thu, May 30, 2019 at 11:39:23AM -0400, Magnus Hagander wrote:
> Congratulations to David, may the buildfarm be gentle to him, and his first
> revert far away!

Congrats!
--
Michael


signature.asc
Description: PGP signature

Re: Why does pg_checksums -r not have a long option?

[Michael Paquier]

On Mon, May 27, 2019 at 04:22:37PM +0200, Fabien COELHO wrote:
> Works for me. Doc build is ok as well.

Thanks, committed.
--
Michael


signature.asc
Description: PGP signature

Re: coverage additions

[Michael Paquier]

On Thu, May 30, 2019 at 01:52:20PM -0400, Alvaro Herrera wrote:
> If there are other obvious improvements to be had, please let me know.
> (We have PG_TEST_EXTRA="ssl ldap" currently, do we have any more extra
> tests now?)

You can add kerberos to this list, to give:
PG_TEST_EXTRA='ssl ldap kerberos'
--
Michael


signature.asc
Description: PGP signature

Re: coverage additions

[Tom Lane]

Alvaro Herrera  writes:
> Apparently, for ecpg you have to do "make checktcp" in order for some of
> the tests to run, and "make check-world" doesn't do that.  Not sure
> what's a good fix for this; do we want to add "make -C
> src/interfaces/ecpg/test checktcp" to what "make check-world" does,
> or do we rather what to add checktcp as a dependency of "make check" in
> src/interfaces/ecpg?

> Or do we just not want this test to be run by default, and thus I should
> add "make -C src/interfaces/ecpg/test checktcp" to coverage.pg.org's
> shell script?

I believe it's intentionally not run by default because it opens up
an externally-accessible server port.

regards, tom lane

Re: compiling PL/pgSQL plugin with C++

[Tom Lane]

I wrote:
> I propose that we change src/tools/pginclude/cpluspluscheck so that
> it searches basically everywhere:
 
> -for f in `find src/include src/interfaces/libpq/libpq-fe.h 
> src/interfaces/libpq/libpq-events.h -name '*.h' -print | \
> +for f in `find src contrib -name '*.h' -print | \

After further experimentation with that, it seems like we'll have
to continue to exclude src/bin/pg_dump/*.h from the C++ check.
pg_dump uses "public" and "namespace" as field names in various
structs, both of which are C++ keywords.  Changing these names
would be quite invasive, and at least in the short run I see no
payoff for doing so.

ecpg/preproc/type.h is also using "new" as a field name, but it
looks like there are few enough references that renaming that
field isn't unreasonable.

There are various other minor issues, but they generally look
fixable with little consequence.

regards, tom lane

Re: coverage additions

[Alvaro Herrera]

On 2019-May-30, Tom Lane wrote:

> Alvaro Herrera  writes:
> > I just enabled --enabled-llvm on the coverage reporting machine, which
> > made src/backend/jit/jit.c go from 60/71 % (line/function wise) to 78/85 % 
> > ...
> > and src/backend/jit/llvm from not appearing at all in the report to
> > 78/94 %.  That's a good improvement.
> 
> > If there are other obvious improvements to be had, please let me know.
> 
> I was going to suggest that adding some or all of
> 
> -DCOPY_PARSE_PLAN_TREES
> -DWRITE_READ_PARSE_PLAN_TREES
> -DRAW_EXPRESSION_COVERAGE_TEST
> 
> to your CPPFLAGS might improve the reported coverage in backend/nodes/,
> and perhaps other places.

I did that, and it certainly increased backend/nodes numbers
considerably.  Thanks.

(extensible.c remains at 0% though, as does its companion nodeCustom.c).

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

Re: coverage additions

[Alvaro Herrera]

Apparently, for ecpg you have to do "make checktcp" in order for some of
the tests to run, and "make check-world" doesn't do that.  Not sure
what's a good fix for this; do we want to add "make -C
src/interfaces/ecpg/test checktcp" to what "make check-world" does,
or do we rather what to add checktcp as a dependency of "make check" in
src/interfaces/ecpg?

Or do we just not want this test to be run by default, and thus I should
add "make -C src/interfaces/ecpg/test checktcp" to coverage.pg.org's
shell script?  Maybe all we need is a way to have it run using
the PG_EXTRA_TEST thingy, but I'm not sure how that works ...?

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

Re: New committer: David Rowley

[Joe Conway]

On 5/30/19 11:43 AM, Andres Freund wrote:
> Hi,
> 
> On 2019-05-30 11:39:23 -0400, Magnus Hagander wrote:
>> For those of you that have not read the minutes from the developer meeting
>> ahead of pgcon (can be found at
>> https://wiki.postgresql.org/wiki/PgCon_2019_Developer_Meeting), we'd like
>> to announce here as well that David Rowley has joined the ranks of
>> PostgreSQL committers.
>>
>> Congratulations to David, may the buildfarm be gentle to him, and his first
>> revert far away!
> 
> Congrats!

+1

Congratulations David!

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: New committer: David Rowley

[Larry Rosenman]

On 05/30/2019 10:39 am, Magnus Hagander wrote:

> Hi! 
> 
> For those of you that have not read the minutes from the developer meeting 
> ahead of pgcon (can be found at 
> https://wiki.postgresql.org/wiki/PgCon_2019_Developer_Meeting), we'd like to 
> announce here as well that David Rowley has joined the ranks of PostgreSQL 
> committers. 
> 
> Congratulations to David, may the buildfarm be gentle to him, and his first 
> revert far away!

Congrats! 
-- 
Larry Rosenman http://www.lerctr.org/~ler
Phone: +1 214-642-9640 E-Mail: l...@lerctr.org
US Mail: 5708 Sabbia Dr, Round Rock, TX 78665-2106

Re: New committer: David Rowley

[Amit Kapila]

On Thu, May 30, 2019 at 9:09 PM Magnus Hagander  wrote:
>
> Hi!
>
> For those of you that have not read the minutes from the developer meeting 
> ahead of pgcon (can be found at 
> https://wiki.postgresql.org/wiki/PgCon_2019_Developer_Meeting), we'd like to 
> announce here as well that David Rowley has joined the ranks of PostgreSQL 
> committers.
>
> Congratulations to David, may the buildfarm be gentle to him, and his first 
> revert far away!
>

Congratulation David!

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

tableam.h fails cpluspluscheck

[Tom Lane]

Using Apple's clang as c++ compiler:

In file included from /tmp/cpluspluscheck.KejiIw/test.cpp:3:
./src/include/access/tableam.h:144:16: error: typedef redefinition with 
different types ('void (*)(Relation, HeapTuple, Datum *, bool *, bool, void *)' 
(aka 'void (*)(RelationData *, HeapTupleData *, unsigned long *, bool *, bool, 
void *)') vs 'IndexBuildCallback')
typedef void (*IndexBuildCallback) (Relation index,
   ^
./src/include/access/tableam.h:36:8: note: previous definition is here
struct IndexBuildCallback;
   ^

(there are some cascading errors, but this is the important one)

Kinda looks like you can't get away with using "struct" on a forward
declaration of something that is not actually a struct type.

regards, tom lane

coverage additions

[Alvaro Herrera]

I just enabled --enabled-llvm on the coverage reporting machine, which
made src/backend/jit/jit.c go from 60/71 % (line/function wise) to 78/85 % ...
and src/backend/jit/llvm from not appearing at all in the report to
78/94 %.  That's a good improvement.

If there are other obvious improvements to be had, please let me know.
(We have PG_TEST_EXTRA="ssl ldap" currently, do we have any more extra
tests now?)

-- 
Álvaro Herrera

Applicant for Google Season of Documentation

[Devansh Gupta]

Hi,

I, Devansh Gupta, have just completed my sophomore year in B.Tech. in
Computer Science and Engineering from International Institute of
Information Technology Hyderabad, India and am planning to contribute to
the documentation of the *postgreSQL *project.

I have already used numPy for many projects and have gone through its
documentation to implement the same. I am also well versed with the
languages used for developing the project and also have the experience in
documentation as part of different assignments and internships.

Since GSoD is relatively newer, I need guidance to know where to start
from. Is there any task that I can perform to know the project better as
well as to develop and showcase the necessary skills?

Thanks and regards

Re: coverage increase for worker_spi

[Tom Lane]

Alvaro Herrera  writes:
> On 2019-May-30, Tom Lane wrote:
>> Hm, I don't understand how this works at all:
>> 
>> +PERFORM pg_sleep(CASE WHEN count(*) = 0 THEN 0 ELSE 0.1 
>> END)
>> +FROM schema1.counted WHERE type = 'delta';
>> +GET DIAGNOSTICS count = ROW_COUNT;
>> 
>> Given that it uses an aggregate, the ROW_COUNT must always be 1, no?

> Well, I was surprised to see the count(*) work there as an argument for
> pg_sleep there at all frankly (maybe we are sleeping 0.1s more than we
> really need, per your observation), but the row_count is concerned with
> rows that have type = 'delta', which are deleted by the bgworker.  So
> the test script job is done when the bgworker has run once through its
> loop.

No, the row_count is going to report the number of rows returned by
the aggregate query, which is going to be one row, independently
of how many rows went into the aggregate.

regression=# do $$
declare c int;
begin
perform count(*) from tenk1;
get diagnostics c = row_count;
raise notice 'c = %', c;
end$$;
psql: NOTICE:  c = 1
DO
regression=# do $$
declare c int;
begin
perform count(*) from tenk1 where false;
get diagnostics c = row_count;
raise notice 'c = %', c;
end$$;
psql: NOTICE:  c = 1
DO

I think you want to capture the actual aggregate output rather than
relying on row_count:

regression=# do $$
declare c int;
begin
c := count(*) from tenk1;
raise notice 'c = %', c;
end$$;
psql: NOTICE:  c = 1
DO
regression=# do $$
declare c int;
begin
c := count(*) from tenk1 where false;
raise notice 'c = %', c;
end$$;
psql: NOTICE:  c = 0
DO

regards, tom lane

Re: New committer: David Rowley

[David Fetter]

On Thu, May 30, 2019 at 11:39:23AM -0400, Magnus Hagander wrote:
> Hi!
> 
> For those of you that have not read the minutes from the developer meeting
> ahead of pgcon (can be found at
> https://wiki.postgresql.org/wiki/PgCon_2019_Developer_Meeting), we'd like
> to announce here as well that David Rowley has joined the ranks of
> PostgreSQL committers.
> 
> Congratulations to David, may the buildfarm be gentle to him, and his first
> revert far away!

Kudos, David!

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

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

Re: New committer: David Rowley

[Andres Freund]

Hi,

On 2019-05-30 11:39:23 -0400, Magnus Hagander wrote:
> For those of you that have not read the minutes from the developer meeting
> ahead of pgcon (can be found at
> https://wiki.postgresql.org/wiki/PgCon_2019_Developer_Meeting), we'd like
> to announce here as well that David Rowley has joined the ranks of
> PostgreSQL committers.
> 
> Congratulations to David, may the buildfarm be gentle to him, and his first
> revert far away!

Congrats!

Greetings,

Andres Freund

Re: coverage increase for worker_spi

[Alvaro Herrera]

On 2019-May-30, Tom Lane wrote:

> Alvaro Herrera  writes:
> > On 2019-May-29, Tom Lane wrote:
> >> I'm not opposed to adding a new test case at this point in the cycle,
> >> but as written this one seems more or less guaranteed to fail under
> >> load.
> 
> > True.  Here's a version that should be more resilient.
> 
> Hm, I don't understand how this works at all:
> 
> + PERFORM pg_sleep(CASE WHEN count(*) = 0 THEN 0 ELSE 0.1 
> END)
> + FROM schema1.counted WHERE type = 'delta';
> + GET DIAGNOSTICS count = ROW_COUNT;
> 
> Given that it uses an aggregate, the ROW_COUNT must always be 1, no?

Well, I was surprised to see the count(*) work there as an argument for
pg_sleep there at all frankly (maybe we are sleeping 0.1s more than we
really need, per your observation), but the row_count is concerned with
rows that have type = 'delta', which are deleted by the bgworker.  So
the test script job is done when the bgworker has run once through its
loop.

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

Re: coverage increase for worker_spi

[Tom Lane]

Alvaro Herrera  writes:
> On 2019-May-29, Tom Lane wrote:
>> I'm not opposed to adding a new test case at this point in the cycle,
>> but as written this one seems more or less guaranteed to fail under
>> load.

> True.  Here's a version that should be more resilient.

Hm, I don't understand how this works at all:

+   PERFORM pg_sleep(CASE WHEN count(*) = 0 THEN 0 ELSE 0.1 
END)
+   FROM schema1.counted WHERE type = 'delta';
+   GET DIAGNOSTICS count = ROW_COUNT;

Given that it uses an aggregate, the ROW_COUNT must always be 1, no?

regards, tom lane

Re: New committer: David Rowley

[David Steele]

On 5/30/19 11:39 AM, Magnus Hagander wrote:


For those of you that have not read the minutes from the developer 
meeting ahead of pgcon (can be found at 
https://wiki.postgresql.org/wiki/PgCon_2019_Developer_Meeting), we'd 
like to announce here as well that David Rowley has joined the ranks of 
PostgreSQL committers.


Congratulations to David, may the buildfarm be gentle to him, and his 
first revert far away!


Congratulations! Very well deserved!

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

Re: error messages in extended statistics

[Alvaro Herrera]

On 2019-May-30, Tomas Vondra wrote:

> Pushed and backpatched, changing most places to elog().

Thanks :-)

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

Re: coverage increase for worker_spi

[Alvaro Herrera]

On 2019-May-29, Tom Lane wrote:

> Alvaro Herrera  writes:
> > Tom pointed out that coverage for worker_spi is 0%.  For a module that
> > only exists to provide coverage, that's pretty stupid.  This patch
> > increases coverage to 90.9% line-wise and 100% function-wise, which
> > seems like a sufficient starting point.
> 
> > How would people feel about me getting this in master at this point in
> > the cycle, it being just some test code?  We can easily revert if
> > it seems too unstable.
> 
> I'm not opposed to adding a new test case at this point in the cycle,
> but as written this one seems more or less guaranteed to fail under
> load.

True.  Here's a version that should be more resilient.

One thing I noticed while writing it, though, is that worker_spi uses
the postgres database, instead of the contrib_regression database that
was created for it.  And we create a schema and a table there.  This is
going to get some eyebrows raised, I think, so I'll look into fixing
that as a bugfix before getting this commit in.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/test/modules/worker_spi/Makefile b/src/test/modules/worker_spi/Makefile
index 7cdb33c9df..cbf9b2e37f 100644
--- a/src/test/modules/worker_spi/Makefile
+++ b/src/test/modules/worker_spi/Makefile
@@ -6,6 +6,14 @@ EXTENSION = worker_spi
 DATA = worker_spi--1.0.sql
 PGFILEDESC = "worker_spi - background worker example"
 
+REGRESS = worker_spi
+
+# enable our module in shared_preload_libraries for dynamic bgworkers
+REGRESS_OPTS = --temp-config $(top_srcdir)/src/test/modules/worker_spi/dynamic.conf
+
+# Disable installcheck to ensure we cover dynamic bgworkers.
+NO_INSTALLCHECK = 1
+
 ifdef USE_PGXS
 PG_CONFIG = pg_config
 PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/src/test/modules/worker_spi/dynamic.conf b/src/test/modules/worker_spi/dynamic.conf
new file mode 100644
index 00..646885a9c7
--- /dev/null
+++ b/src/test/modules/worker_spi/dynamic.conf
@@ -0,0 +1,2 @@
+worker_spi.naptime = 1
+shared_preload_libraries = worker_spi
diff --git a/src/test/modules/worker_spi/expected/worker_spi.out b/src/test/modules/worker_spi/expected/worker_spi.out
new file mode 100644
index 00..cf1f252e29
--- /dev/null
+++ b/src/test/modules/worker_spi/expected/worker_spi.out
@@ -0,0 +1,37 @@
+\c postgres
+CREATE EXTENSION worker_spi;
+SELECT worker_spi_launch(1) IS NOT NULL;
+ ?column? 
+--
+ t
+(1 row)
+
+INSERT INTO schema1.counted VALUES ('total', 0), ('delta', 1);
+SELECT pg_reload_conf();
+ pg_reload_conf 
+
+ t
+(1 row)
+
+DO $$
+	DECLARE
+		count int;
+		loops int := 0;
+	BEGIN
+		LOOP
+			PERFORM pg_sleep(CASE WHEN count(*) = 0 THEN 0 ELSE 0.1 END)
+			FROM schema1.counted WHERE type = 'delta';
+			GET DIAGNOSTICS count = ROW_COUNT;
+			loops := loops + 1;
+			IF count < 1 OR loops > 180 * 10 THEN
+RETURN;
+			END IF;
+	END LOOP;
+END
+$$;
+SELECT * FROM schema1.counted;
+ type  | value 
+---+---
+ total | 1
+(1 row)
+
diff --git a/src/test/modules/worker_spi/sql/worker_spi.sql b/src/test/modules/worker_spi/sql/worker_spi.sql
new file mode 100644
index 00..bae2680dfb
--- /dev/null
+++ b/src/test/modules/worker_spi/sql/worker_spi.sql
@@ -0,0 +1,22 @@
+\c postgres
+CREATE EXTENSION worker_spi;
+SELECT worker_spi_launch(1) IS NOT NULL;
+INSERT INTO schema1.counted VALUES ('total', 0), ('delta', 1);
+SELECT pg_reload_conf();
+DO $$
+	DECLARE
+		count int;
+		loops int := 0;
+	BEGIN
+		LOOP
+			PERFORM pg_sleep(CASE WHEN count(*) = 0 THEN 0 ELSE 0.1 END)
+			FROM schema1.counted WHERE type = 'delta';
+			GET DIAGNOSTICS count = ROW_COUNT;
+			loops := loops + 1;
+			IF count < 1 OR loops > 180 * 10 THEN
+RETURN;
+			END IF;
+	END LOOP;
+END
+$$;
+SELECT * FROM schema1.counted;
-- 
2.17.1

Re: compiling PL/pgSQL plugin with C++

[Tom Lane]

[ redirecting to -hackers ]

=?koi8-r?B?9MHSwdPP1yDnxc/Sx8nKIPfJ1MHM2MXXyd4=?=  writes:
> I'm working on development of some PL/pgSQL plugin.
> The smaller part of my code is written on C.
> It's a standard extension code for integration with fmgr (_PG_init ...)
> But bigger part of the code is written on C++. 
> And here I need declarations of internal PL/pgSQL structs from plpgsql.h

So ... that's supposed to work, because we have a test script that
verifies that all our headers compile as C++.

Or I thought it was "all", anyway.  Closer inspection shows that it's
not checking src/pl.  Nor contrib.

I propose that we change src/tools/pginclude/cpluspluscheck so that
it searches basically everywhere:
 
-for f in `find src/include src/interfaces/libpq/libpq-fe.h 
src/interfaces/libpq/libpq-events.h -name '*.h' -print | \
+for f in `find src contrib -name '*.h' -print | \

However, trying to run that, I find that plpython and plperl are both
seriously in violation of the project convention that headers should
compile standalone.  It looks like most of their headers rely on an
assumption that the calling .c file already included the Python or
Perl headers respectively.

Anybody object to me reshuffling the #include's to make this pass?
I propose doing that for HEAD only, although we should back-patch
George's fix (and any other actual C++ problems we find).

regards, tom lane

Re: New committer: David Rowley

[Marko Tiikkaja]

On Thu, May 30, 2019 at 6:39 PM Magnus Hagander  wrote:
For those of you that have not read the minutes from the developer meeting
ahead of pgcon (can be found at
https://wiki.postgresql.org/wiki/PgCon_2019_Developer_Meeting), we'd like
to announce here as well that David Rowley has joined the ranks of
PostgreSQL committers.

Congratulations to David, may the buildfarm be gentle to him, and his first
revert far away!

Yee.

>

New committer: David Rowley

[Magnus Hagander]

Hi!

For those of you that have not read the minutes from the developer meeting
ahead of pgcon (can be found at
https://wiki.postgresql.org/wiki/PgCon_2019_Developer_Meeting), we'd like
to announce here as well that David Rowley has joined the ranks of
PostgreSQL committers.

Congratulations to David, may the buildfarm be gentle to him, and his first
revert far away!

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

Re: error messages in extended statistics

[Tomas Vondra]

On Wed, May 15, 2019 at 06:35:47PM +0200, Tomas Vondra wrote:

On Wed, May 15, 2019 at 12:17:29PM -0400, Alvaro Herrera wrote:

On 2019-May-05, Tomas Vondra wrote:


OK, so here is a patch, using elog() for all places except for the
input function, where we simply report we don't accept those values.


Hmm, does this actually work?  I didn't know that elog() supported
errcode()/errmsg()/etc.  I thought the macro definition didn't allow for
that.



D'oh, it probably does not. I might not have tried to compile it before
sending it to the mailing list, not sure ... :-(


Anyway, since the messages are still passed with errmsg(), they would
still end up in the message catalog, so this patch doesn't help my case.
I would suggest that instead of changing ereport to elog, you should
change errmsg() to errmsg_internal().  That prevents the translation
marking, and achieves the desired effect.  (You can verify by running
"make update-po" in src/backend/ and seeing that the msgid no longer
appears in postgres.pot).


Now, what about backpatch? It's a small tweak, but it makes the life a
bit easier for translators ...


+1 for backpatching.



Pushed and backpatched, changing most places to elog().

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

Re: Zedstore - compressed in-core columnar storage

[DEV_OPS]

it's really cool and very good progress, 

I'm interesting if SIDM/JIT will be supported

best wishes

TY


On 2019/5/23 08:07, Ashwin Agrawal wrote:
> We (Heikki, me and Melanie) are continuing to build Zedstore. Wish to
> share the recent additions and modifications. Attaching a patch
> with the latest code. Link to github branch [1] to follow
> along. The approach we have been leaning towards is to build required
> functionality, get passing the test and then continue to iterate to
> optimize the same. It's still work-in-progress.
>
> Sharing the details now, as have reached our next milestone for
> Zedstore. All table AM API's are implemented for Zedstore (except
> compute_xid_horizon_for_tuples, seems need test for it first).
>
> Current State:
>
> - A new type of item added to Zedstore "Array item", to boost
>   compression and performance. Based on Konstantin's performance
>   experiments [2] and inputs from Tomas Vodra [3], this is
>   added. Array item holds multiple datums, with consecutive TIDs and
>   the same visibility information. An array item saves space compared
>   to multiple single items, by leaving out repetitive UNDO and TID
>   fields. An array item cannot mix NULLs and non-NULLs. So, those
>   experiments should result in improved performance now. Inserting
>   data via COPY creates array items currently. Code for insert has not
>   been modified from last time. Making singleton inserts or insert
>   into select, performant is still on the todo list.
>
> - Now we have a separate and dedicated meta-column btree alongside
>   rest of the data column btrees. This special or first btree for
>   meta-column is used to assign TIDs for tuples, track the UNDO
>   location which provides visibility information. Also, this special
>   btree, which always exists, helps to support zero-column tables
>   (which can be a result of ADD COLUMN DROP COLUMN actions as
>   well). Plus, having meta-data stored separately from data, helps to
>   get better compression ratios. And also helps to further simplify
>   the overall design/implementation as for deletes just need to edit
>   the meta-column and avoid touching the actual data btrees. Index
>   scans can just perform visibility checks based on this meta-column
>   and fetch required datums only for visible tuples. For tuple locks
>   also just need to access this meta-column only. Previously, every
>   column btree used to carry the same undo pointer. Thus visibility
>   check could be potentially performed, with the past layout, using
>   any column. But considering overall simplification new layout
>   provides it's fine to give up on that aspect. Having dedicated
>   meta-column highly simplified handling for add columns with default
>   and null values, as this column deterministically provides all the
>   TIDs present in the table, which can't be said for any other data
>   columns due to default or null values during add column.
>
> - Free Page Map implemented. The Free Page Map keeps track of unused
>   pages in the relation. The FPM is also a b-tree, indexed by physical
>   block number. To be more compact, it stores "extents", i.e. block
>   ranges, rather than just blocks, when possible. An interesting paper [4]
> on
>   how modern filesystems manage space acted as a good source for ideas.
>
> - Tuple locks implemented
>
> - Serializable isolation handled
>
> - With "default_table_access_method=zedstore"
>   - 31 out of 194 failing regress tests
>   - 10 out of 86 failing isolation tests
> Many of the current failing tests are due to plan differences, like
> Index scans selected for zedstore over IndexOnly scans, as zedstore
> doesn't yet have visibility map. I am yet to give a thought on
> index-only scans. Or plan diffs due to table size differences between
> heap and zedstore.
>
> Next few milestones we wish to hit for Zedstore:
> - Make check regress green
> - Make check isolation green
> - Zedstore crash safe (means also replication safe). Implement WAL
>   logs
> - Performance profiling and optimizations for Insert, Selects, Index
>   Scans, etc...
> - Once UNDO framework lands in Upstream, Zedstore leverages it instead
>   of its own version of UNDO
>
> Open questions / discussion items:
>
> - how best to get "column projection list" from planner? (currently,
>   we walk plan and find the columns required for the query in
>   the executor, refer GetNeededColumnsForNode())
>
> - how to pass the "column projection list" to table AM? (as stated in
>   initial email, currently we have modified table am API to pass the
>   projection to AM)
>
> - TID treated as (block, offset) in current indexing code
>
> - Physical tlist optimization? (currently, we disabled it for
>   zedstore)
>
> Team:
> Melanie joined Heikki and me to write code for zedstore. Majority of
> the code continues to be contributed by Heikki. We are continuing to
> have fun building column store implementation and iterate
> aggressively.
>
> References:
> 1] 

Re: Server crash due to assertion failure in CheckOpSlotCompatibility()

[Andres Freund]

Hi,


On 2019-05-30 16:31:39 +0530, Ashutosh Sharma wrote:
> Here are some more details on the crash reported in my previous e-mail for
> better clarity:

I'll look into this once pgcon is over... Thanks for finding!

Greetings,

Andres Freund

Re: Minimal logical decoding on standbys

[Andres Freund]

Hi,

On 2019-05-30 19:46:26 +0530, Amit Khandekar wrote:
> @@ -1042,7 +1042,8 @@ ReplicationSlotReserveWal(void)
>  if (!RecoveryInProgress() && SlotIsLogical(slot))
>  {
> 
>  }
>  else
>  {
> -   restart_lsn = GetRedoRecPtr();
> +   restart_lsn = SlotIsLogical(slot) ?
> +GetXLogReplayRecPtr() : 
> GetRedoRecPtr();
> 
> But then when I do pg_create_logical_replication_slot(), it hangs in
> DecodingContextFindStartpoint(), waiting to find new records
> (XLogReadRecord).

But just till the primary has logged the necessary WAL records? If you
just do CHECKPOINT; or such on the primary, it should succeed quickly?

Greetings,

Andres Freund

Re: [HACKERS] [PATCH] Generic type subscripting

[Dmitry Dolgov]

> On Wed, May 29, 2019 at 6:17 PM Pavel Stehule  wrote:
>
> st 29. 5. 2019 v 17:49 odesílatel Dmitry Dolgov <9erthali...@gmail.com> 
> napsal:
>>
>> Rebase after pg_indent. Besides, off the list there was a suggestion that 
>> this
>> could be useful to accept more than one data type as a key for subscripting.
>> E.g. for jsonb it probably makes sense to understand both a simple key name 
>> and
>> jsonpath:
>>
>> jsonb['a'] and jsonb['$.a']
>>
>> While to implement it can be technically relatively straightforward I guess, 
>> I
>> wonder if there is any opinion about how valuable it could be and what it
>> should looks like from the syntax point of view (since I believe a user needs
>> to specify which type needs to be used).
>
>
> It is difficult decision - possibility to use jsonpath looks great, but
> necessity to cast every time is not friendly.

Thanks. Yes, I also wonder if it's possible to avoid type casting every time,
but other ideas seems syntactically equally not friendly.

> Probably there can be preferred type if subscripting is of unknown type.
> There can be similar rules to function's parameters.
>
> so jsonb['a'] -- key
> jsonb['$.a'] -- key
> jsonb['$.a'::jsonpath'] -- json path
>
> but it can be source of bad issues - so I think we don't need this feature in
> this moment. This feature can be implemented later, I think.

Yeah, I agree it's something that looks like a good potential improvement, not
now but in the future.

Re: Minimal logical decoding on standbys

[Amit Khandekar]

On Mon, 27 May 2019 at 19:26, Andres Freund  wrote:
>
> On 2019-05-27 17:04:44 +0530, Amit Khandekar wrote:
> > On Fri, 24 May 2019 at 21:00, Amit Khandekar  wrote:
> > >
> > > On Fri, 24 May 2019 at 19:26, Amit Khandekar  
> > > wrote:
> > > > Working on the patch now 
> > >
> > > Attached is an incremental WIP patch
> > > handle_wal_level_changes_WIP.patch to be applied over the earlier main
> > > patch logical-decoding-on-standby_v4_rebased.patch.
> >
> > I found an issue with these changes : When we change master wal_level
> > from logical to hot_standby, and again back to logical, and then
> > create a logical replication slot on slave, it gets created; but when
> > I do pg_logical_slot_get_changes() with that slot, it seems to read
> > records *before* I created the logical slot, so it encounters
> > parameter-change(logical=>hot_standby) record, so returns an error as
> > per the patch, because now in DecodeXLogOp() I error out when
> > XLOG_PARAMETER_CHANGE is found :
>
>
> > @@ -190,11 +190,23 @@ DecodeXLogOp(LogicalDecodingContext *ctx,
> > XLogRecordBuffer *buf)
> >  * can restart from there.
> >  */
> > break;
> > + case XLOG_PARAMETER_CHANGE:
> > + {
> > +   xl_parameter_change *xlrec =
> > + (xl_parameter_change *) XLogRecGetData(buf->record);
> > +
> > +   /* Cannot proceed if master itself does not have logical data */
> > +   if (xlrec->wal_level < WAL_LEVEL_LOGICAL)
> > + ereport(ERROR,
> > + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> > +  errmsg("logical decoding on standby requires "
> > + "wal_level >= logical on master")));
> > +   break;
> > + }
> >
> > I thought it won't read records *before* the slot was created. Am I
> > missing something ?
>
> That's why I had mentioned that you'd need to adapt
> ReplicationSlotReserveWal(), to use the replay LSN or such.

Yeah ok. I tried to do this :

@@ -1042,7 +1042,8 @@ ReplicationSlotReserveWal(void)
 if (!RecoveryInProgress() && SlotIsLogical(slot))
 {

 }
 else
 {
-   restart_lsn = GetRedoRecPtr();
+   restart_lsn = SlotIsLogical(slot) ?
+GetXLogReplayRecPtr() : GetRedoRecPtr();

But then when I do pg_create_logical_replication_slot(), it hangs in
DecodingContextFindStartpoint(), waiting to find new records
(XLogReadRecord).

Working on it ...



--
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company

Re: Server crash due to assertion failure in CheckOpSlotCompatibility()

[Ashutosh Sharma]

Hi All,

Here are some more details on the crash reported in my previous e-mail for
better clarity:

The crash only happens when a *primary key* or *btree index* is created on
the test table. For example consider the following two scenarios.

*TC1: With PK*
create table t1(a int *primary key*, b text);
insert into t1 values (1, 'aa'), (2, 'bb'), (3, 'aa'), (4, 'bb');
select a, b, array_agg(a order by a) from t1 group by grouping sets ((a),
(b));

This (TC1) is the problematic case, the explain plan for the query causing
the crash is as follows

postgres=# explain select a, b, array_agg(a order by a) from t1 group by
grouping sets ((a), (b));
 QUERY PLAN

-
 GroupAggregate  (cost=0.15..166.92 rows=1470 width=68)
   Group Key: a
   Sort Key: b
 Group Key: b
   ->  Index Scan using t1_pkey on t1  (cost=0.15..67.20 rows=1270 width=36)
(5 rows)

*TC2: Without PK/Btree index*
create table t2(a int, b text);
insert into t2 values (1, 'aa'), (2, 'bb'), (3, 'aa'), (4, 'bb');
select a, b, array_agg(a order by a) from t2 group by grouping sets ((a),
(b));

And here is the explain plan for the query in TC2 that doesn't cause any
crash

postgres=# explain select a, b, array_agg(a order by a) from t2 group by
grouping sets ((a), (b));
QUERY PLAN
---
 GroupAggregate  (cost=88.17..177.69 rows=400 width=68)
   Group Key: a
   Sort Key: b
 Group Key: b
   ->  Sort  (cost=88.17..91.35 rows=1270 width=36)
 *Sort Key: a*
 ->  Seq Scan on t2  (cost=0.00..22.70 rows=1270 width=36)
(7 rows)

If you notice the difference between the two plans, in case of TC1, the
Index Scan was performed on the test table and as the data in the index
(btree index) is already sorted, when grouping aggregate is performed on
the column 'a', there is *no* sorting done for it (you would see that "*Sort
Key: a*" is missing in the explain plan for TC1)and for that reason it
expects the slot to contain the heap tuple but then, as the slots are
fetched from the tuplesort object, it actually contains minimal tuple. On
the other hand, if you see the explain plan for TC2, the sorting is done
for both the groups (i.e. both "Sort Key: b" && "Sort Key: a" exists) and
hence the expected slot is always the minimal slot so there is no assertion
failure in case 2.

Thanks,

-- 
With Regards,
Ashutosh Sharma
EnterpriseDB:*http://www.enterprisedb.com *

On Wed, May 29, 2019 at 5:50 PM Ashutosh Sharma 
wrote:

> Hi All,
>
> I'm getting a server crash when executing the following test-case:
>
> create table t1(a int primary key, b text);
> insert into t1 values (1, 'aa'), (2, 'bb'), (3, 'aa'), (4, 'bb');
> select a, b, array_agg(a order by a) from t1 group by grouping sets ((a),
> (b));
>
> *Backtrace:*
> #0  0x7f37d0630277 in raise () from /lib64/libc.so.6
> #1  0x7f37d0631968 in abort () from /lib64/libc.so.6
> #2  0x00a5685e in ExceptionalCondition (conditionName=0xc29fd0
> "!(op->d.fetch.kind == slot->tts_ops)", errorType=0xc29cc1
> "FailedAssertion",
> fileName=0xc29d09 "execExprInterp.c", lineNumber=1905) at assert.c:54
> #3  0x006dfa2b in CheckOpSlotCompatibility (op=0x2e84e38,
> slot=0x2e6e268) at execExprInterp.c:1905
> #4  0x006dd446 in ExecInterpExpr (state=0x2e84da0,
> econtext=0x2e6d8e8, isnull=0x7ffe53cba4af) at execExprInterp.c:439
> #5  0x007010e5 in ExecEvalExprSwitchContext (state=0x2e84da0,
> econtext=0x2e6d8e8, isNull=0x7ffe53cba4af)
> at ../../../src/include/executor/executor.h:307
> #6  0x00701be7 in advance_aggregates (aggstate=0x2e6d6b0) at
> nodeAgg.c:679
> #7  0x00703a5d in agg_retrieve_direct (aggstate=0x2e6d6b0) at
> nodeAgg.c:1847
> #8  0x007034da in ExecAgg (pstate=0x2e6d6b0) at nodeAgg.c:1572
> #9  0x006e797f in ExecProcNode (node=0x2e6d6b0) at
> ../../../src/include/executor/executor.h:239
> #10 0x006ea174 in ExecutePlan (estate=0x2e6d458,
> planstate=0x2e6d6b0, use_parallel_mode=false, operation=CMD_SELECT,
> sendTuples=true,
> numberTuples=0, direction=ForwardScanDirection, dest=0x2e76b30,
> execute_once=true) at execMain.c:1648
> #11 0x006e7f91 in standard_ExecutorRun (queryDesc=0x2e7b3b8,
> direction=ForwardScanDirection, count=0, execute_once=true) at
> execMain.c:365
> #12 0x006e7dc7 in ExecutorRun (queryDesc=0x2e7b3b8,
> direction=ForwardScanDirection, count=0, execute_once=true) at
> execMain.c:309
> #13 0x008e40c7 in PortalRunSelect (portal=0x2e10bc8, forward=true,
> count=0, dest=0x2e76b30) at pquery.c:929
> #14 0x008e3d66 in PortalRun (portal=0x2e10bc8,
> count=9223372036854775807, isTopLevel=true, run_once=true, dest=0x2e76b30,
> altdest=0x2e76b30,
> completionTag=0x7ffe53cba850 "") at pquery.c:770
>
> The following Assert statement in 

Re: Fix inconsistencies for v12

[Amit Kapila]

On Wed, May 29, 2019 at 6:12 AM Amit Langote
 wrote:
>
> On 2019/05/28 20:26, Amit Kapila wrote:
> > On Tue, May 28, 2019 at 12:29 PM Amit Langote wrote:
> >> Seeing that the crash occurs due to postgres_fdw relying on
> >> es_result_relation_info being set when initializing a "direct
> >> modification" plan on foreign tables managed by it, we could change the
> >> comment to say that instead.  Note that allowing "direct modification" of
> >> foreign tables is a core feature, so there's no postgres_fdw-specific
> >> behavior here; there may be other FDWs that support "direct modification"
> >> plans and so likewise rely on es_result_relation_info being set.
> >
> >
> > Can we ensure some way that only FDW's rely on it not any other part
> > of the code?
>
> Hmm, I can't think of any way of doing than other than manual inspection.
> We are sure that no piece of core code relies on it in the ExecInitNode()
> code path.  Apparently FDWs may, as we've found out here.  Now that I've
> looked around, maybe other loadable modules may too, by way of (only?)
> Custom nodes.  I don't see any other way to hook into ExecInitNode(), so
> maybe that's it.
>
> So, maybe reword a bit as:
>
> diff --git a/src/backend/executor/nodeModifyTable.c
> b/src/backend/executor/nodeModifyTable.c
> index a3c0e91543..95545c9472 100644
> --- a/src/backend/executor/nodeModifyTable.c
> +++ b/src/backend/executor/nodeModifyTable.c
> @@ -2316,7 +2316,7 @@ ExecInitModifyTable(ModifyTable *node, EState
> *estate, int eflags)
>   * verify that the proposed target relations are valid and open their
>   * indexes for insertion of new index entries.  Note we *must* set
>   * estate->es_result_relation_info correctly while we initialize each
> - * sub-plan; ExecContextForcesOids depends on that!
> + * sub-plan; external modules such as FDWs may depend on that.
>

I think it will be better to include postgres_fdw in the comment in
some way so that if someone wants a concrete example, there is
something to refer to.

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

Re: Fix inconsistencies for v12

[Amit Kapila]

On Tue, May 28, 2019 at 10:30 AM Alexander Lakhin  wrote:
>
> 28.05.2019 2:05, Amit Kapila wrote:
> > On Mon, May 27, 2019 at 3:59 AM Tom Lane  wrote:
> >> Amit Kapila  writes:
> >>> On Sun, May 26, 2019 at 2:20 AM Alexander Lakhin  
> >>> wrote:
>  5. ExecContextForcesOids - not changed, but may be should be removed
>  (orphaned after 578b2297)
> >>> Yes, we should remove the use of ExecContextForcesOids.
> >> Unless grep is failing me, ExecContextForcesOids is in fact gone.
> >> All that's left is one obsolete mention in a comment, which should
> >> certainly be cleaned up.
> >>
..
> > */
> I got a coredump with `make installcheck-world` (on postgres_fdw test):
>

Thanks for noticing this.  I have run the tests in parallel mode with
something like make -s check-world -j4 PROVE_FLAGS='-j4'.  It didn't
stop at failure, so I missed to notice it.  However, now looking
carefully (by redirecting the output to a log file), I could see this.

>
> (On a side note, I agree with your remarks regarding 2 and 3; please
> look at a better patch for 3 attached.)
>

The new patch looks good to me.  However, instead of committing just
this one alone, I will review others as well and see which all can be
combined and pushed together.

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

Re: How to know referenced sub-fields of a composite type?

[Amit Langote]

On 2019/05/29 15:50, Kohei KaiGai wrote:
> 2019年5月29日(水) 13:26 Amit Langote :
>>> It means we can skip to load the sub-fields unreferenced, if
>>> query-planner can handle
>>> referenced and unreferenced sub-fields correctly.
>>> On the other hands, it looks to me RelOptInfo or other optimizer
>>> related structure don't have
>>> this kind of information. RelOptInfo->attr_needed tells extension
>>> which attributes are referenced
>>> by other relation, however, its granularity is not sufficient for 
>>> sub-fields.
>>
>> Isn't that true for some other cases as well, like when a query accesses
>> only some sub-fields of a json(b) column?  In that case too, planner
>> itself can't optimize away access to other sub-fields.  What it can do
>> though is match a suitable index to the operator used to access the
>> individual sub-fields, so that the index (if one is matched and chosen)
>> can optimize away accessing unnecessary sub-fields.  IOW, it seems to me
>> that the optimizer leaves it up to the indexes (and plan nodes) to further
>> optimize access to within a field.  How is this case any different?
>
> I think it is a little bit different scenario.
> Even if an index on sub-fields can indicate the tuples to be fetched,
> the fetched tuple contains all the sub-fields because heaptuple is
> row-oriented data.
>
> For example, if WHERE-clause checks a sub-field: "x" then aggregate
> function references other sub-field "y", Scan/Join node has to return
> a tuple that contains both "x" and "y". IndexScan also pops up a tuple
> with a full composite type, so here is no problem if we cannot know
> which sub-fields are referenced in the later stage.
> Maybe, if IndexOnlyScan supports to return a partial composite type,
> it needs similar infrastructure that can be used for a better composite
> type support on columnar storage.

Ah indeed.  I think I had misunderstood your intent.  Indexes have to do
with optimizing the "filtering" of complex/nested type (json, Arrow
Struct, etc.) values, where unnecessary sub-fields need not be read before
filtering, whereas you're interested in optimizing "projections" of
complex types, where sub-fields that are not used anywhere in the query
need not be read from the stored values.

>>> Probably, all we can do right now is walk-on the RelOptInfo list to
>>> lookup FieldSelect node
>>> to see the referenced sub-fields. Do we have a good idea instead of
>>> this expensive way?
>>> # Right now, PG-Strom loads all the sub-fields of Struct column from
>>> arrow_fdw foreign-table
>>> # regardless of referenced / unreferenced sub-fields. Just a second best.
>>
>> I'm missing something, but if PG-Strom/arrow_fdw does look at the
>> FieldSelect nodes to see which sub-fields are referenced, why doesn't it
>> generate a plan that will only access those sub-fields or why can't it?
>>
> Likely, it is not a technical problem but not a smart implementation.
> If I missed some existing infrastructure we can apply, it may be more suitable
> than query/expression tree walking.

There is no infrastructure for this as far as I know.  Maybe, some will be
built in the future now that storage format is pluggable.

Thanks,
Amit

Re: Pinned files at Windows

[Konstantin Knizhnik]

On 29.05.2019 22:20, Michael Paquier wrote:

On Mon, May 27, 2019 at 05:52:13PM +0300, Konstantin Knizhnik wrote:

Postgres is opening file with FILE_SHARE_DELETE  flag which makes it
possible to unlink opened file.
But unlike Unixes, the file is not actually deleted. You can see it using
"dir" command.
And stat() function also doesn't return error in this case:

https://stackoverflow.com/questions/27270374/deletefile-or-unlink-calls-succeed-but-doesnt-remove-file

So first check in  pgwin32_safestat (r < 0) is not working at all: stat()
returns 0, but subsequent call of GetFileAttributesEx
returns 5 (ERROR_ACCESS_DENIED).

So you would basically hijack the result of GetFileAttributesEx() so
as any errors returned by this function complain with ENOENT for
everything seen.  Why would that be a sane idea?  What if say a
permission or another error is legit, but instead ENOENT is returned
as you propose, then the caller would be confused by an incorrect
status.


If access to the file is prohibited by lack of permissions, then stat() 
should fail with error

and this error is returned by  pgwin32_safestat function.

If call of stat() is succeed, then my assumption is that the only reason 
of GetFileAttributesEx
failure is that file is deleted and returning ENOENT error code in this 
case is correct behavior.




As you mention, what we did as of 9951741 may not be completely right,
and the reason why it was done this way comes from here:
https://www.postgresql.org/message-id/20160712083220.1426.58...@wrigleys.postgresql.org


Yes, this is the same reason, but handling STATUS_DELETE_PENDING is not 
correct.


Could we instead come up with a reliable way to detect if a file is in
a deletion pending state?  Mapping blindly EACCES to ENOENT is not a
solution I think we can rely on (perhaps we could check only after
ERROR_ACCESS_DENIED using GetLastError() and map back to ENOENT in
this case still this can be triggered if a virus scanner holds the
file for read, no?).  stat() returning 0 for a file pending for
deletion which will go away physically once the handles still keeping
the file around are closed is not something I would have imagined is
sane, but that's what we need to deal with...  Windows has a long
history of keeping things compatible, sometimes in their own weird
way, and it seems that we have one here so I cannot imagine that this
behavior is going to change.

Looking around, I have found out about NtCreateFile() which could be
able to report a proper pending deletion status, still that's only
available in kernel mode.  Perhaps others have ideas?


Sorry, I do not know better solution.
I have written small test reproducing the problem which proves that
if file is opened with FILE_SHARE_DELETE flag, then
it is possible to delete it using unlink() - no error is returned and 
call stat() for it - also succeed.
By any attempt to open this file for reading/writing or performing 
GetFileAttributesEx
are failed with  ERROR_ACCESS_DENIED (not with ERROR_DELETE_PENDING 
which is hidden by Win32 API).


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