Re: [HACKERS] UPDATE of partition key

[David Rowley]

 On 10 November 2017 at 16:42, Amit Khandekar  wrote:
[ update-partition-key_v23.patch ]

Hi Amit,

Thanks for working on this. I'm looking forward to seeing this go in.

So... I've signed myself up to review the patch, and I've just had a
look at it, (after first reading this entire email thread!).

Overall the patch looks like it's in quite a good shape. I think I do
agree with Robert about the UPDATE anomaly that's been discussed. I
don't think we're painting ourselves into any corner by not having
this working correctly right away. Anyone who's using some trigger
workarounds for the current lack of support for updating the partition
key is already going to have the same issues, so at least this will
save them some troubles implementing triggers and give them much
better performance. I see you've documented this fact too, which is
good.

I'm writing this email now as I've just run out of review time for today.

Here's what I noted down during my first pass:

1. Closing command tags in docs should not be abbreviated

triggers are concerned, AFTER DELETE and

This changed in c29c5789. I think Peter will be happy if you don't
abbreviate the closing tags.

2. "about to do" would read better as "about to perform"

 concurrent session, and it is about to do an UPDATE

I think this paragraph could be more clear if we identified the
sessions with a number.

Perhaps:
   Suppose, session 1 is performing an UPDATE on a
   partition key, meanwhile, session 2 tries to perform an UPDATE
or DELETE operation on the same row.
   Session 2 can silently miss the row due to session 1's activity.  In
   such a case, session 2's UPDATE/DELETE
   , being unaware of the row's movement, interprets this that the
   row has just been deleted, so there is nothing to be done for this row.
   Whereas, in the usual case where the table is not partitioned, or where
   there is no row movement, the second session would have identified the
   newly updated row and carried UPDATE/DELETE
on this new row version.


3. Integer width. get_partition_natts returns int but we assign to int16.

int16 partnatts = get_partition_natts(key);

Confusingly get_partition_col_attnum() returns int16 instead of AttrNumber
but that's existingly not correct.

4. The following code could just pull_varattnos(partexprs, 1, _keycols);

foreach(lc, partexprs)
{
Node*expr = (Node *) lfirst(lc);

pull_varattnos(expr, 1, _keycols);
}

5. Triggers. Do we need a new "TG_" tag to allow trigger functions to
do something
special when the DELETE/INSERT is a partition move? I have audit
tables in mind here
it may appear as though a user performed a DELETE when they actually
performed an UPDATE
giving visibility of this to the trigger function will allow the
application to work around this.

6. change "row" to "a row" and "old" to "the old"

* depending on whether the event is for row being deleted from old

But to be honest, I'm having trouble parsing the comment. I think it
would be better to
say explicitly when the row will be NULL rather than "depending on
whether the event"

7. I'm confused with how this change came about. If the old comment
was correct here then the comment you're referring to here should
remain in ExecPartitionCheck(), but you're saying it's in
ExecConstraints().

/* See the comments in ExecConstraints. */

If the comment really is in ExecConstraints(), then you might want to
give an overview of what you mean, then reference ExecConstraints() if
more details are required.

8. I'm having trouble parsing this comment:

 * 'update_rri' has the UPDATE per-subplan result rels.

I think "has" should be "contains" ?

9. Also, this should likely be reworded:

 * 'num_update_rri' : number of UPDATE per-subplan result rels. For INSERT,
 *  this is 0.

 'num_update_rri' number of elements in 'update_rri' array or zero for INSERT.

10. There should be no space before the '?'

/* Is this leaf partition present in the update resultrel ? */

11. I'm struggling to understand this comment:

* This is required when converting tuple as per root
* partition tuple descriptor.

"tuple" should probably be "the tuple", but not quite sure what you
mean by "as per root".

I may have misunderstood, but maybe it should read:

* This is required when we convert the partition's tuple to
* be compatible with the partitioned table's tuple descriptor.

12. I think "as well" would be better written as "either".

* If we didn't open the partition rel, it means we haven't
* initialized the result rel as well.

13. I'm unsure what is meant by the following comment:

* Verify result relation is a valid target for insert operation. Even
* for updates, we are doing this for tuple-routing, so again, we need
* to check the validity for insert operation.

I'm not quite sure where UPDATE comes in here as we're only checking for INSERT?

14. Use of underscores instead of camelCase.

Re: [HACKERS] Parallel Hash take II

[Thomas Munro]

Hi Andres and Peter,

Please see below for inline responses to your feedback.  New patch attached.

On Wed, Nov 8, 2017 at 10:01 AM, Andres Freund  wrote:
> +set min_parallel_table_scan_size = 0;
> +set parallel_setup_cost = 0;
> +-- Make a simple relation with well distributed keys and correctly
> +-- estimated size.
> +create table simple as
> +  select generate_series(1, 2) AS id, 
> 'aa';
> +alter table simple set (parallel_workers = 2);
> +analyze simple;
> +-- Make a relation whose size we will under-estimate.  We want stats
> +-- to say 1000 rows, but actually there are 20,000 rows.
> +create table bigger_than_it_looks as
> +  select generate_series(1, 2) as id, 
> 'aa';
> +alter table bigger_than_it_looks set (autovacuum_enabled = 'false');
> +alter table bigger_than_it_looks set (parallel_workers = 2);
> +delete from bigger_than_it_looks where id <= 19000;
> +vacuum bigger_than_it_looks;
> +analyze bigger_than_it_looks;
> +insert into bigger_than_it_looks
> +  select generate_series(1, 19000) as id, 
> 'aa';
>
> It seems kinda easier to just manipulate ndistinct and reltuples...

Done.

> +set max_parallel_workers_per_gather = 0;
> +set work_mem = '4MB';
>
> I hope there's a fair amount of slop here - with different archs you're
> going to see quite some size differences.

Yeah, this is a problem I wrestled with.  See next.

> +-- The "good" case: batches required, but we plan the right number; we
> +-- plan for 16 batches, and we stick to that number, and peak memory
> +-- usage says within our work_mem budget
> +-- non-parallel
> +set max_parallel_workers_per_gather = 0;
> +set work_mem = '128kB';
>
> So how do we know that's actually the case we're testing rather than
> something arbitrarily different? There's IIRC tests somewhere that just
> filter the json explain output to the right parts...

Yeah, good idea.  My earlier attempts to dump out the hash join
dimensions ran into problems with architecture sensitivity and then
some run-to-run non-determinism in the parallel case (due to varying
fragmentation depending on how many workers get involved in time).
The attached version tells you about batch growth without reporting
the exact numbers, except in the "ugly" case where we know that there
is only one possible outcome because the extreme skew detector is
guaranteed to go off after the first nbatch increase (I got rid of all
other tuples except ones with the same key to make this true).

This exercise did reveal a bug in
0008-Show-hash-join-per-worker-information-in-EXPLAIN-ANA.patch
though: it is capturing shared instrumentation too soon in the
non-Parallel Hash case so the nbatch reported by EXPLAIN ANALYZE might
be too low if we grew while probing.  Oops.  Will post a fix for that.

> +/*
> + * Build the name for a given segment of a given BufFile.
> + */
> +static void
> +MakeSharedSegmentName(char *name, const char *buffile_name, int segment)
> +{
> +   snprintf(name, MAXPGPATH, "%s.%d", buffile_name, segment);
> +}
>
> Not a fan of this name - you're not "making" a filename here (as in
> allocating or such). I think I'd just remove the Make prefix.

Done.  I also changed some similar code where I'd used GetXXX when
building paths.

> +/*
> + * Open a file that was previously created in another backend with
> + * BufFileCreateShared in the same SharedFileSet using the same name.  The
> + * backend that created the file must have called BufFileClose() or
> + * BufFileExport() to make sure that it is ready to be opened by other
> + * backends and render it read-only.
> + */
>
> Is it actually guaranteed that it's another backend / do we rely on
> that?

No, it could be any backend that is attached to the SharedFileSet,
including the current one.  Wording improved.

> +BufFile *
> +BufFileOpenShared(SharedFileSet *fileset, const char *name)
> +{
>
> +   /*
> +* If we didn't find any files at all, then no BufFile exists with 
> this
> +* tag.
> +*/
> +   if (nfiles == 0)
> +   return NULL;
>
> s/taag/name/?

Fixed.

> +/*
> + * Delete a BufFile that was created by BufFileCreateShared in the given
> + * SharedFileSet using the given name.
> + *
> + * It is not necessary to delete files explicitly with this function.  It is
> + * provided only as a way to delete files proactively, rather than waiting 
> for
> + * the SharedFileSet to be cleaned up.
> + *
> + * Only one backend should attempt to delete a given name, and should know
> + * that it exists and has been exported or closed.
> + */
> +void
> +BufFileDeleteShared(SharedFileSet *fileset, const char *name)
> +{
> +   charsegment_name[MAXPGPATH];
> +   int segment = 0;
> +   boolfound = false;
> +
> +   /*
> +* We don't know how many segments the file has.  We'll keep deleting
> +* until we 

Re: [HACKERS] proposal: schema variables

[Pavel Stehule]

Hi

2017-11-13 13:15 GMT+01:00 Pavel Golub :

> Hello, Pavel.
>
> You wrote:
>
> PS> Hi,
>
> PS> I propose a  new database object - a variable. The variable is
> PS> persistent object, that holds unshared session based not
> PS> transactional in memory value of any type. Like variables in any
> PS> other languages. The persistence is required for possibility to do
> PS> static checks, but can be limited to session - the variables can be
> temporal.
>
> Great idea.
>
> PS> My proposal is related to session variables from Sybase, MSSQL or
> PS> MySQL (based on prefix usage @ or @@), or package variables from
> PS> Oracle (access is controlled by scope), or schema variables from
> PS> DB2. Any design is coming from different sources, traditions and
> PS> has some advantages or disadvantages. The base of my proposal is
> PS> usage schema variables as session variables for stored procedures.
> PS> It should to help to people who try to port complex projects to
> PostgreSQL from other databases.
>
> PS> The Sybase  (T-SQL) design is good for interactive work, but it
> PS> is weak for usage in stored procedures - the static check is not
> PS> possible. Is not possible to set some access rights on variables.
>
> PS> The ADA design (used on Oracle) based on scope is great, but our
> PS> environment is not nested. And we should to support other PL than
> PLpgSQL more strongly.
>
> PS> There is not too much other possibilities - the variable that
> PS> should be accessed from different PL, different procedures (in
> PS> time) should to live somewhere over PL, and there is the schema only.
>
> PS> The variable can be created by CREATE statement:
>
> PS> CREATE VARIABLE public.myvar AS integer;
> PS> CREATE VARIABLE myschema.myvar AS mytype;
>
> PS> CREATE [TEMP] VARIABLE [IF NOT EXISTS] name AS type
> PS>   [ DEFAULT expression ] [[NOT] NULL]
> PS>   [ ON TRANSACTION END { RESET | DROP } ]
> PS>   [ { VOLATILE | STABLE } ];
>
>
> PS> It is dropped by command DROP VARIABLE  [ IF EXISTS] varname.
>
> PS> The access rights is controlled by usual access rights - by
> PS> commands GRANT/REVOKE. The possible rights are: READ, WRITE
>
> PS> The variables can be modified by SQL command SET (this is taken from
> standard, and it natural)
>
> PS> SET varname = expression;
>
> I propose LET keyword for this to distinguish GUC from variables, e.g.
>
> LET varname = expression;
>

 It is one possible variant. I plan to implement more variants and then
choose one.

Regards

Pavel

>
> PS> Unfortunately we use the SET command for different purpose. But I
> PS> am thinking so we can solve it with few tricks. The first is
> PS> moving our GUC to pg_catalog schema. We can control the strictness
> PS> of SET command. In one variant, we can detect custom GUC and allow
> PS> it, in another we can disallow a custom GUC and allow only schema
> PS> variables. A new command LET can be alternative.
>
>
>
> PS> The variables should be used in queries implicitly (without JOIN)
>
>
> PS> SELECT varname;
>
>
> PS> The SEARCH_PATH is used, when varname is located. The variables
> PS> can be used everywhere where query parameters are allowed.
>
>
>
> PS> I hope so this proposal is good enough and simple.
>
>
> PS> Comments, notes?
>
>
> PS> regards
>
>
> PS> Pavel
>
>
>
>
>
>
> --
> With best wishes,
>  Pavel  mailto:pa...@gf.microolap.com
>
>

Re: [HACKERS] proposal: schema variables

[Pavel Golub]

Hello, Pavel.

You wrote:

PS> Hi,

PS> I propose a  new database object - a variable. The variable is
PS> persistent object, that holds unshared session based not
PS> transactional in memory value of any type. Like variables in any
PS> other languages. The persistence is required for possibility to do
PS> static checks, but can be limited to session - the variables can be 
temporal.

Great idea.

PS> My proposal is related to session variables from Sybase, MSSQL or
PS> MySQL (based on prefix usage @ or @@), or package variables from
PS> Oracle (access is controlled by scope), or schema variables from
PS> DB2. Any design is coming from different sources, traditions and
PS> has some advantages or disadvantages. The base of my proposal is
PS> usage schema variables as session variables for stored procedures.
PS> It should to help to people who try to port complex projects to PostgreSQL 
from other databases.

PS> The Sybase  (T-SQL) design is good for interactive work, but it
PS> is weak for usage in stored procedures - the static check is not
PS> possible. Is not possible to set some access rights on variables.

PS> The ADA design (used on Oracle) based on scope is great, but our
PS> environment is not nested. And we should to support other PL than PLpgSQL 
more strongly.

PS> There is not too much other possibilities - the variable that
PS> should be accessed from different PL, different procedures (in
PS> time) should to live somewhere over PL, and there is the schema only.

PS> The variable can be created by CREATE statement:

PS> CREATE VARIABLE public.myvar AS integer;
PS> CREATE VARIABLE myschema.myvar AS mytype;

PS> CREATE [TEMP] VARIABLE [IF NOT EXISTS] name AS type 
PS>   [ DEFAULT expression ] [[NOT] NULL]
PS>   [ ON TRANSACTION END { RESET | DROP } ]
PS>   [ { VOLATILE | STABLE } ];


PS> It is dropped by command DROP VARIABLE  [ IF EXISTS] varname.

PS> The access rights is controlled by usual access rights - by
PS> commands GRANT/REVOKE. The possible rights are: READ, WRITE

PS> The variables can be modified by SQL command SET (this is taken from 
standard, and it natural)

PS> SET varname = expression;

I propose LET keyword for this to distinguish GUC from variables, e.g.

LET varname = expression;

PS> Unfortunately we use the SET command for different purpose. But I
PS> am thinking so we can solve it with few tricks. The first is
PS> moving our GUC to pg_catalog schema. We can control the strictness
PS> of SET command. In one variant, we can detect custom GUC and allow
PS> it, in another we can disallow a custom GUC and allow only schema
PS> variables. A new command LET can be alternative.



PS> The variables should be used in queries implicitly (without JOIN)


PS> SELECT varname;


PS> The SEARCH_PATH is used, when varname is located. The variables
PS> can be used everywhere where query parameters are allowed. 



PS> I hope so this proposal is good enough and simple.


PS> Comments, notes?


PS> regards


PS> Pavel






-- 
With best wishes,
 Pavel  mailto:pa...@gf.microolap.com



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

Re: [HACKERS] Fix bloom WAL tap test

[Alexander Korotkov]

Hi!

On Fri, Nov 10, 2017 at 9:12 PM, Tom Lane  wrote:

> I wrote:
> > Is there anything we can do to cut the runtime of the TAP test to
> > the point where running it by default wouldn't be so painful?
>
> As an experiment, I tried simply cutting the size of the test table 10X:
>
> diff --git a/contrib/bloom/t/001_wal.pl b/contrib/bloom/t/001_wal.pl
> index 1b319c9..566abf9 100644
> --- a/contrib/bloom/t/001_wal.pl
> +++ b/contrib/bloom/t/001_wal.pl
> @@ -57,7 +57,7 @@ $node_standby->start;
>  $node_master->safe_psql("postgres", "CREATE EXTENSION bloom;");
>  $node_master->safe_psql("postgres", "CREATE TABLE tst (i int4, t
> text);");
>  $node_master->safe_psql("postgres",
> -"INSERT INTO tst SELECT i%10, substr(md5(i::text), 1, 1) FROM
> generate_series(1,10) i;"
> +"INSERT INTO tst SELECT i%10, substr(md5(i::text), 1, 1) FROM
> generate_series(1,1) i;"
>  );
>  $node_master->safe_psql("postgres",
> "CREATE INDEX bloomidx ON tst USING bloom (i, t) WITH (col1 =
> 3);");
> @@ -72,7 +72,7 @@ for my $i (1 .. 10)
> test_index_replay("delete $i");
> $node_master->safe_psql("postgres", "VACUUM tst;");
> test_index_replay("vacuum $i");
> -   my ($start, $end) = (11 + ($i - 1) * 1, 10 + $i *
> 1);
> +   my ($start, $end) = (10001 + ($i - 1) * 1000, 1 + $i * 1000);
> $node_master->safe_psql("postgres",
>  "INSERT INTO tst SELECT i%10, substr(md5(i::text), 1, 1) FROM
> generate_series($start,$end) i;"
> );
>
> This about halved the runtime of the TAP test, and it changed the coverage
> footprint not at all according to lcov.  (Said coverage is only marginally
> better than what we get without running the bloom TAP test, AFAICT.)
>
> It seems like some effort could be put into both shortening this test
> and improving the amount of code it exercises.
>

Thank you for committing patch which fixes tap test.
I'll try to improve coverage of this test and reduce its run time.

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

Re: [HACKERS] How to implement a SP-GiST index as a extension module?

[Alexander Korotkov]

Hi!

On Mon, Nov 13, 2017 at 6:47 AM, Connor Wolf  wrote:

> Ok, I've managed to get my custom index working.
>

Good!

It's all on github here: https://github.com/fake-name/pg-spgist_hamming, if
> anyone else needs a fuzzy-image searching system
> that can integrate into postgresql..
>
> It should be a pretty good basis for anyone else to use if they want to
> implement a SP-GiST index too.
>

I took a look at the code, and I feel myself a bit confused :)
It appears that you're indexing int8 values.  That seems like unrealistic
short representation for image signature.
Also, name of repository make me think that hamming distance would be used
to compare signatures.  But after look at the code, I see that plain
absolute value of difference is used for that purpose.

static double
getDistance(Datum v1, Datum v2)
{
int64_t a1 = DatumGetInt64(v1);
int64_t a2 = DatumGetInt64(v2);
int64_t diff = Abs(a1 - a2);
fprintf_to_ereport("getDistance %ld <-> %ld : %ld", a1, a2, diff);
return diff;
}

For such notion of distance, you don't need a VP-tree or another complex
indexing.  B-tree is quite enough in this case.  Alternatively, distance
function is not what it meant to be.

It would be useful if you provide complete usage example of this extension:
from image to signature conversion to search queries.

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

Re: [HACKERS] Jsonb transform for pl/python

[Anthony Bykov]

On Thu, 09 Nov 2017 12:26:46 +
Aleksander Alekseev  wrote:

> The following review has been posted through the commitfest
> application: make installcheck-world:  tested, failed
> Implements feature:   tested, passed
> Spec compliant:   tested, passed
> Documentation:tested, passed
> 
> Hello Anthony,
> 
> Great job!
> 
> I decided to take a closer look on your patch. Here are some defects
> I discovered.
> 
> > +   Additional extensions are available that implement transforms
> > for
> > +   the jsonb type for the language PL/Python.  The
> > +   extensions for PL/Perl are called  
> 
> 1. The part regarding PL/Perl is obviously from another patch.
> 
> 2. jsonb_plpython2u and jsonb_plpythonu are marked as relocatable,
> while jsonb_plpython3u is not. Is it a mistake? Anyway if an
> extension is relocatable there should be a test that checks this.
> 
> 3. Not all json types are test-covered. Tests for 'true' :: jsonb,
> '3.14' :: jsonb and 'null' :: jsonb are missing.
> 
> 4. jsonb_plpython.c:133 - "Iterate trhrough Jsonb object." Typo, it
> should be "through" or probably even "over".
> 
> 5. It looks like you've implemented transform in two directions
> Python <-> JSONB, however I see tests only for Python <- JSONB case.
> 
> 6. Tests passed on Python 2.7.14 but failed on 3.6.2:
> 
> >  CREATE EXTENSION jsonb_plpython3u CASCADE;
> > + ERROR:  could not access file "$libdir/jsonb_plpython3": No such
> > file or directory  
> 
> module_pathname in jsonb_plpython3u.control should be
> $libdir/jsonb_plpython3u, not $libdir/jsonb_plpython3.
> 
> Tested on Arch Linux x64, GCC 7.2.0.
> 
> The new status of this patch is: Waiting on Author
> 

Hello, Aleksander. 
Thank you for your time. The defects you have noticed were fixed.
Please, find in attachments new version of the patch (it is called
0001-jsonb_plpython-extension-v2.patch).

Most of changes were made to fix defects(list of the defects may be
found in citation in the beginning of this message), but the algorithm
of iterating through incoming jsonb was changed so that it looks tidier.

--
Anthony Bykov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Companydiff --git a/contrib/Makefile b/contrib/Makefile
index 8046ca4..d9d9817 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -81,9 +81,9 @@ ALWAYS_SUBDIRS += hstore_plperl
 endif
 
 ifeq ($(with_python),yes)
-SUBDIRS += hstore_plpython ltree_plpython
+SUBDIRS += hstore_plpython ltree_plpython jsonb_plpython
 else
-ALWAYS_SUBDIRS += hstore_plpython ltree_plpython
+ALWAYS_SUBDIRS += hstore_plpython ltree_plpython jsonb_plpython
 endif
 
 # Missing:
diff --git a/contrib/jsonb_plpython/Makefile b/contrib/jsonb_plpython/Makefile
new file mode 100644
index 000..6371d11
--- /dev/null
+++ b/contrib/jsonb_plpython/Makefile
@@ -0,0 +1,39 @@
+# contrib/jsonb_plpython/Makefile
+
+MODULE_big = jsonb_plpython$(python_majorversion)u
+OBJS = jsonb_plpython.o $(WIN32RES)
+PGFILEDESC = "jsonb_plpython - transform between jsonb and plpythonu"
+
+PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plpython $(python_includespec) -DPLPYTHON_LIBNAME='"plpython$(python_majorversion)"'
+
+EXTENSION = jsonb_plpythonu jsonb_plpython2u jsonb_plpython3u
+DATA = jsonb_plpythonu--1.0.sql jsonb_plpython2u--1.0.sql jsonb_plpython3u--1.0.sql
+
+REGRESS = jsonb_plpython$(python_majorversion)
+REGRESS_PLPYTHON3_MANGLE := $(REGRESS)
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = contrib/jsonb_plpython
+top_builddir = ../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
+
+# We must link libpython explicitly
+ifeq ($(PORTNAME), win32)
+# ... see silliness in plpython Makefile ...
+SHLIB_LINK += $(sort $(wildcard ../../src/pl/plpython/libpython*.a))
+else
+rpathdir = $(python_libdir)
+SHLIB_LINK += $(python_libspec) $(python_additional_libs)
+endif
+
+ifeq ($(python_majorversion),2)
+REGRESS_OPTS += --load-extension=plpython2u
+else
+REGRESS_OPTS += --load-extension=plpython3u
+endif
diff --git a/contrib/jsonb_plpython/expected/jsonb_plpython2.out b/contrib/jsonb_plpython/expected/jsonb_plpython2.out
new file mode 100644
index 000..8ad5338
--- /dev/null
+++ b/contrib/jsonb_plpython/expected/jsonb_plpython2.out
@@ -0,0 +1,478 @@
+CREATE EXTENSION jsonb_plpython2u CASCADE;
+-- test jsonb -> python dict
+CREATE FUNCTION test1(val jsonb) RETURNS int
+LANGUAGE plpython2u
+TRANSFORM FOR TYPE jsonb
+AS $$
+assert isinstance(val, dict)
+plpy.info(sorted(val.items()))
+return len(val)
+$$;
+SELECT test1('{"a":1, "c":"NULL"}'::jsonb);
+INFO:  [('a', Decimal('1')), ('c', 'NULL')]
+ test1 
+---
+ 2
+(1 row)
+
+-- test jsonb -> python dict
+-- complex dict with dicts as value
+CREATE FUNCTION test1complex(val jsonb) RETURNS int
+LANGUAGE plpython2u
+TRANSFORM FOR TYPE jsonb
+AS $$
+assert isinstance(val, dict)
+assert(val == {"d":{"d": 1}})
+return 

Re: [HACKERS] path toward faster partition pruning

[Amit Langote]

Horiguchi-san,

Thanks for taking a look.  Replying to all your emails here.

On 2017/11/10 12:30, Kyotaro HORIGUCHI wrote:
> In 0002, bms_add_range has a bit naive-looking loop
> 
> + while (wordnum <= uwordnum)
> + {
> + bitmapword mask = (bitmapword) ~0;
> +
> + /* If working on the lower word, zero out bits below 'lower'. */
> + if (wordnum == lwordnum)
> + {
> + int lbitnum = BITNUM(lower);
> + mask >>= lbitnum;
> + mask <<= lbitnum;
> + }
> +
> + /* Likewise, if working on the upper word, zero bits above 
> 'upper' */
> + if (wordnum == uwordnum)
> + {
> + int ushiftbits = BITS_PER_BITMAPWORD - (BITNUM(upper) + 
> 1);
> + mask <<= ushiftbits;
> + mask >>= ushiftbits;
> + }
> +
> + a->words[wordnum++] |= mask;
> + }
> 
> Without some aggressive optimization, the loop takes most of the
> time to check-and-jump for nothing especially with many
> partitions and somewhat unintuitive.
> 
> The following uses a bit tricky bitmap operation but
> is straightforward as a whole.
> 
> =
> /* fill the bits upper from BITNUM(lower) (0-based) of the first word */
> a->workds[wordnum++] += ~(bitmapword)((1 << BITNUM(lower)) - 1);
> 
> /* fill up intermediate words */
> while (wordnum < uwordnum)
>a->words[wordnum++] = ~(bitmapword) 0;
> 
> /* fill up to BITNUM(upper) bit (0-based) of the last word */
> a->workds[wordnum++] |=
>  (~(bitmapword) 0) >> (BITS_PER_BITMAPWORD - (BITNUM(upper) - 1));
> =

Considering also the David's comment downthread, I will try to incorporate
this into bms_add_range().

> In 0003, 
> 
> +match_clauses_to_partkey(RelOptInfo *rel,
> ...
> +  if (rinfo->pseudoconstant &&
> +(IsA(clause, Const) &&
> + Const *) clause)->constisnull) ||
> +  !DatumGetBool(((Const *) clause)->constvalue
> +  {
> +*constfalse = true;
> +continue;
> 
> If we call this function in both conjunction and disjunction
> context (the latter is only in recursive case). constfalse ==
> true means no need of any clauses for the former case.
> 
> Since (I think) just a list of RestrictInfo is expected to be
> treated as a conjunction (it's quite doubious, though..),

I think it makes sense to consider a list of RestrictInfo's, such as
baserestrictinfo, that is passed as input to match_clauses_to_partkey(),
to be mutually conjunctive for our purpose here.

> we
> might be better call this for each subnodes of a disjunction. Or,
> like match_clauses_to_index, we might be better provide
> match_clause_to_partkey(rel, rinfo, contains_const), which
> returns NULL if constfalse. (I'm not self-confident on this..)

After reading your comment, I realized that it was wrong that the
recursive call to match_clauses_to_partkey() passed the arguments of an OR
clause all at once.  That broke the assumption mentioned above that all of
the clauses in the list passed to match_clauses_to_partkey() are mutually
conjunctive.  Instead, we must make a single-member list for each of the
OR clause's arguments and pass the same.

Then if we got constfalse for all of the OR's arguments, then we return
the constfalse=true to the original caller.

> 
> +  /*
> +   * If no commutator exists, cannot flip the qual's args,
> +   * so give up.
> +   */
> +  if (!OidIsValid(expr_op))
> +continue;
> 
> I suppose it's better to leftop and rightop as is rather than
> flipping over so that var is placed left-side. Does that make
> things so complex?

Reason to do it that way is that the code placed far away (code in
partition.c that extracts constant values to use for pruning from matched
clauses) can always assume that the clauses determined to be useful for
partition-pruning always come in the 'partkey op constant' form.

> + * It the operator happens to be '<>', which is never listed
> If?

Right, will fix.

> +if (!op_in_opfamily(expr_op, partopfamily))
> +{
> +  Oidnegator = get_negator(expr_op);
> +
> +  if (!OidIsValid(negator) ||
> +!op_in_opfamily(negator, partopfamily))
> +continue;
> 
> classify_partition_bounding_keys() checks the same thing by
> checking whether the negator's strategy is
> BTEquealStrategyNumber. (I'm not sure the operator is guaranteed
> to be of btreee, though..) Aren't they needed to be in similar
> way?

You're right.  The <>'s negator may not always be a btree operator.  So,
we should add a check in match_clauses_to_partkey() that list or range
partitioning is in use, because only those require a btree operator
family.  We now have hash partitioning, so need to be careful not to make
the assumption that all partitioning operators are from btree operator
families.

If match_clauses_to_partkey() 

Re: [HACKERS] 10beta1 sequence regression failure on sparc64

[Jonathan Jacobson]

Christoph, what beta2 change was it that fixed that problem?

I'm having exactly the same regression test failure in version 10.1 (not beta) 
running on Solaris 9

Compiler used: GCC 4.6.4
CFLAGS: -O2 -m64


On 13/07/2017 20:05, Christoph Berg wrote:

Re: To Andres Freund 2017-05-24 <20170524170921.7pykzbt54dlfk...@msg.df7cb.de>

If we had a typo or something in that code, the build farm should have
caught it by now.

I would try compiling with lower -O and see what happens.

Trying -O0 now.

Sorry for the late reply, I was hoping to catch you on IRC, but then
didn't manage to be around at a time that would fit the timezone
shift.

The -O0 build passed the regression tests. Not sure what that means
for our problem, though.

Fwiw, the problem is fixed with beta2, even with -O2:

https://buildd.debian.org/status/logs.php?pkg=postgresql-10=sparc64

Christoph






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

Re: [HACKERS] [Patch] Log SSL certificate verification errors

[Laurenz Albe]

Graham Leggett wrote:
> Currently neither the server side nor the client side SSL certificate verify
> callback does anything, leading to potential hair-tearing-out moments.
>
> The following patch to master implements logging of all certificate
> verification failures, as well as (crucially) which certificates failed to 
> verify,
> and at what depth, so the admin can zoom in straight onto the problem without 
> any guessing.

+1 for the idea.

I have been in this situation before, and any information that helps to
clarify what the problem is would be a great help.

Yours,
Laurenz Albe


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

Re: [HACKERS] Partition-wise aggregation/grouping

[Konstantin Knizhnik]

On 11.11.2017 23:29, Konstantin Knizhnik wrote:

On 10/27/2017 02:01 PM, Jeevan Chalke wrote:

Hi,

Attached new patch-set here. Changes include:

1. Added separate patch for costing Append node as discussed up-front 
in the

patch-set.
2. Since we now cost Append node, we don't need 
partition_wise_agg_cost_factor
GUC. So removed that. The remaining patch hence merged into main 
implementation

patch.
3. Updated rows in test-cases so that we will get partition-wise plans.

Thanks


I applied partition-wise-agg-v6.tar.gz patch to  the master and use 
shard.sh example from 
https://www.postgresql.org/message-id/14577.1509723225%40localhost

Plan for count(*) is the following:

shard=# explain select count(*) from orders;
  QUERY PLAN
--- 


 Finalize Aggregate  (cost=100415.29..100415.30 rows=1 width=8)
   ->  Append  (cost=50207.63..100415.29 rows=2 width=8)
 ->  Partial Aggregate  (cost=50207.63..50207.64 rows=1 width=8)
   ->  Foreign Scan on orders_0 (cost=101.00..50195.13 
rows=5000 width=0)

 ->  Partial Aggregate  (cost=50207.63..50207.64 rows=1 width=8)
   ->  Foreign Scan on orders_1 (cost=101.00..50195.13 
rows=5000 width=0)



We really calculate partial aggregate for each partition, but to do we 
still have to fetch all data from remote host.

So for foreign partitions such plans is absolutely inefficient.
Amy be it should be combined with some other patch?
For example, with  agg_pushdown_v4.tgz patch 
https://www.postgresql.org/message-id/14577.1509723225%40localhost ?

But it is not applied after partition-wise-agg-v6.tar.gz patch.
Also postgres_fdw in 11dev is able to push down aggregates without 
agg_pushdown_v4.tgz patch.


In 0009-Teach-postgres_fdw-to-push-aggregates-for-child-rela.patch
there is the following check:

 /* Partial aggregates are not supported. */
+   if (extra->isPartial)
+   return;

If we just comment this line then produced plan will be the following:

shard=# explain select sum(product_id) from orders;
   QUERY PLAN

 Finalize Aggregate  (cost=308.41..308.42 rows=1 width=8)
   ->  Append  (cost=144.18..308.41 rows=2 width=8)
 ->  Foreign Scan  (cost=144.18..154.20 rows=1 width=8)
   Relations: Aggregate on (public.orders_0 orders)
 ->  Foreign Scan  (cost=144.18..154.20 rows=1 width=8)
   Relations: Aggregate on (public.orders_1 orders)
(6 rows)

And it is actually desired plan!
Obviously such approach will not always work. FDW really doesn't 
support partial aggregates now.
But for most frequently used aggregates: sum, min, max, count 
aggtype==aggtranstype and there is no difference

between partial and normal aggregate calculation.
So instead of (extra->isPartial) condition we can add more complex 
check which will traverse pathtarget expressions and
check if it can be evaluated in this way. Or... extend FDW API to 
support partial aggregation.


But even the last plan is not ideal: it will calculate predicates at 
each remote node sequentially.

There is parallel append patch:
https://www.postgresql.org/message-id/CAJ3gD9ctEcrVUmpY6fq_JUB6WDKGXAGd70EY68jVFA4kxMbKeQ%40mail.gmail.com 

but ... FDW doesn't support parallel scan, so parallel append can not 
be applied in this case.
And we actually do not need parallel append with all its dynamic 
workers here.
We just need to start commands at all remote servers and only after it 
fetch results (which can be done sequentially).


I am investigating problem of efficient execution of OLAP queries on 
sharded tables (tables with remote partitions).

After reading all this threads and corresponding  patches, it seems to me
that we already have most of parts of the puzzle, what we need is to 
put them on right places and may be add missed ones.

I wonder if somebody is busy with it and can I somehow help here?

Also I am not quite sure about the best approach with parallel 
execution of distributed query at all nodes.
Should we make postgres_fdw parallel safe and use parallel append? How 
difficult it will be?
Or in addition to parallel append we should also have "asynchronous 
append" which will be able to initiate execution at all nodes?
It seems to be close to merge append, because it should simultaneously 
traverse all cursors.


Looks like second approach is easier for implementation. But in case 
of sharded table, distributed query may need to traverse both remote
and local shards and this approach doesn't allow to processed several 
local shards in parallel.




I attach small patch for postgres_fdw.c which allows concurrent 
execution of aggregates by all remote servers (when them are accessed 
through postgres_fdw).
I have added "postgres_fdw.use_prefetch" GUC to enable/disable 
prefetching data in postgres_fdw.

Re: [HACKERS] index-only count(*) for indexes supporting bitmap scans

[Alexander Kuzmenkov]

I've pushed the executor part of this, but mostly not the planner part,
because I didn't think the latter was anywhere near ready for prime
time: the regression test changes it was causing were entirely bogus.


Hi Tom,

Thanks for the commit and the explanation. I'll try to address your 
comments if I continue working on the planner part.


--

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



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

Re: [HACKERS] Building PL/Perl with ActiveState Perl 5.22 and MSVC

[Noah Misch]

On Sat, Mar 26, 2016 at 03:43:21PM +0300, Victor Wagner wrote:
> It turns out that while ActiveState seems to drop support of embedding
> their perl into msvc-compiled appications, there are just few minor
> issues which prevent PL-perl to compile.
> 
> 1. ActiveState Perl doesn't ship MSVC-build import library perl522.lib
> for their perl522.dll. Instead they ship MINGW-build library
> libperl522.a.
> 
> Visual Studio 2012 is able to link against this library. It is only
> matter of modifing search expresions in Mkvcbuild.pm to be able to find
> this import library. Not sure if it stands true for all eariler
> versions of Visual Studio, supported by Postgresql.
> 
> 2. There is macro PERL_STATIC_INLINME in perl's lib/CORE/config.h file,
> which produces compilation errors.

> #define PERL_STATIC_INLINE static __inline__  /**/

I've seen the same problems, and I converted your description into the
attached patch.  With ActivePerl-5.24.1.2402-MSWin32-x64-401627.exe binaries,
"vcregress plcheck" passes.  I plan to back-patch this.  If some site has
worked around this with "copy libperl522.a perl522.lib", that site's build
will fail due to having two matching libraries.  The build failure and fix
will be clear enough, so that seems acceptable.

> 3. Fixing two issues above was enough to make build complete for 64-bit
> windows target. With 32-bit build I'v got linking errors 
> LINK2026: module unsafe for safeseh image
> 
> for all modules which are linked with perl DLL. I suspect that it is
> not  a problem with DLL itself, it is rather related to way MINGW32
> generates its import libraries. To fix this problem XML element
> false
> should be added inside  element of the two vcxproj files which
> link with perl - plperl.vcxproj and hstore_plperl.vcxproj.

Official 10.1 x86 binaries[1] contain a SAFESEH build of plperl.dll, linked
with a perl524.dll.  I wonder how.  Do they use a perl524.dll from a source
other than ActivePerl?  Before adopting your proposal, I'd like to understand
why the official binaries haven't needed it.  Also, I'd instead use
"", which omits /SAFESEH entirely instead of
passing /SAFESEH:NO.  That way, only the binaries linked to Perl (or other
old-fashioned DLLs) lose their safe exception handler table.

Even if I do this, "vcregress plcheck" fails with "loadable library and perl
binaries are mismatched (got handshake key 0B080080, needed 0AF00080)".
That's building with ActivePerl-5.22.4.2205-MSWin32-x86-64int-403863.exe and
VS2015.  When I have more information, I'll send it to the thread[2] about
that failure mode.

Thanks,
nm

[1] 
https://get.enterprisedb.com/postgresql/postgresql-10.1-1-windows-binaries.zip
[2] 
https://postgr.es/m/flat/CAE9k0P%3DhMFew%3DVxNGTeOJJr32%2BUDy-o2qWXrThHg524EBqvnZQ%40mail.gmail.com
diff --git a/src/pl/plperl/plperl.h b/src/pl/plperl/plperl.h
index c4e06d0..aac95f8 100644
--- a/src/pl/plperl/plperl.h
+++ b/src/pl/plperl/plperl.h
@@ -42,6 +42,14 @@
 #undef vsnprintf
 #endif
 
+/*
+ * ActivePerl 5.18 and later are MinGW-built, and their headers use GCC's
+ * __inline__.  Translate to something MSVC recognizes.
+ */
+#ifdef _MSC_VER
+#define __inline__ inline
+#endif
+
 
 /*
  * Get the basic Perl API.  We use PERL_NO_GET_CONTEXT mode so that our code
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index 686c736..4c2e12e 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -615,9 +615,10 @@ sub mkvcbuild
}
}
$plperl->AddReference($postgres);
-   my $perl_path = $solution->{options}->{perl} . 
'\lib\CORE\perl*.lib';
+   my $perl_path = $solution->{options}->{perl} . 
'\lib\CORE\*perl*';
+   # ActivePerl 5.16 provided perl516.lib; 5.18 provided 
libperl518.a
my @perl_libs =
- grep { /perl\d+.lib$/ } glob($perl_path);
+ grep { /perl\d+\.lib$|libperl\d+\.a$/ } glob($perl_path);
if (@perl_libs == 1)
{
$plperl->AddLibrary($perl_libs[0]);

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

Re: [HACKERS] Parallel Append implementation

[Amit Khandekar]

Thanks a lot Robert for the patch. I will have a look. Quickly tried
to test some aggregate queries with a partitioned pgbench_accounts
table, and it is crashing. Will get back with the fix, and any other
review comments.

Thanks
-Amit Khandekar

On 9 November 2017 at 23:44, Robert Haas  wrote:
> On Sat, Oct 28, 2017 at 5:50 AM, Robert Haas  wrote:
>> No, because the Append node is *NOT* getting copied into shared
>> memory.  I have pushed a comment update to the existing functions; you
>> can use the same comment for this patch.
>
> I spent the last several days working on this patch, which had a
> number of problems both cosmetic and functional.  I think the attached
> is in better shape now, but it could certainly use some more review
> and testing since I only just finished modifying it, and I modified it
> pretty heavily.  Changes:
>
> - I fixed the "morerows" entries in the documentation.  If you had
> built the documentation the way you had it and loaded up in a web
> browser, you would have seen that the way you had it was not correct.
>
> - I moved T_AppendState to a different position in the switch inside
> ExecParallelReInitializeDSM, so as to keep that switch in the same
> order as all of the other switch statements in that file.
>
> - I rewrote the comment for pa_finished.  It previously began with
> "workers currently executing the subplan", which is not an accurate
> description. I suspect this was a holdover from a previous version of
> the patch in which this was an array of integers rather than an array
> of type bool.  I also fixed the comment in ExecAppendEstimate and
> added, removed, or rewrote various other comments as well.
>
> - I renamed PA_INVALID_PLAN to INVALID_SUBPLAN_INDEX, which I think is
> more clear and allows for the possibility that this sentinel value
> might someday be used for non-parallel-aware Append plans.
>
> - I largely rewrote the code for picking the next subplan.  A
> superficial problem with the way that you had it is that you had
> renamed exec_append_initialize_next to exec_append_seq_next but not
> updated the header comment to match.  Also, the logic was spread out
> all over the file.  There are three cases: not parallel aware, leader,
> worker.  You had the code for the first case at the top of the file
> and the other two cases at the bottom of the file and used multiple
> "if" statements to pick the right one in each case.  I replaced all
> that with a function pointer stored in the AppendState, moved the code
> so it's all together, and rewrote it in a way that I find easier to
> understand.  I also changed the naming convention.
>
> - I renamed pappend_len to pstate_len and ParallelAppendDescData to
> ParallelAppendState.  I think the use of the word "descriptor" is a
> carryover from the concept of a scan descriptor.  There's nothing
> really wrong with inventing the concept of an "append descriptor", but
> it seems more clear to just refer to shared state.
>
> - I fixed ExecAppendReInitializeDSM not to reset node->as_whichplan.
> Per commit 41b0dd987d44089dc48e9c70024277e253b396b7, that's wrong;
> instead, local state should be reset in ExecReScanAppend.  I installed
> what I believe to be the correct logic in that function instead.
>
> - I fixed list_qsort() so that it copies the type of the old list into
> the new list.  Otherwise, sorting a list of type T_IntList or
> T_OidList would turn it into just plain T_List, which is wrong.
>
> - I removed get_append_num_workers and integrated the logic into the
> callers.  This function was coded quite strangely: it assigned the
> return value of fls() to a double and then eventually rounded the
> result back to an integer.  But fls() returns an integer, so this
> doesn't make much sense.  On a related note, I made it use fls(# of
> subpaths) instead of fls(# of subpaths)+1.  Adding 1 doesn't make
> sense to me here because it leads to a decision to use 2 workers for a
> single, non-partial subpath.  I suspect both of these mistakes stem
> from thinking that fls() returns the base-2 logarithm, but in fact it
> doesn't, quite: log2(1) = 0.0 but fls(1) = 1.
>
> - In the process of making the changes described in the previous
> point, I added a couple of assertions, one of which promptly failed.
> It turns out the reason is that your patch didn't update
> accumulate_append_subpaths(), which can result in flattening
> non-partial paths from a Parallel Append into a parent Append's list
> of partial paths, which is bad.  The easiest way to fix that would be
> to just teach accumulate_append_subpaths() not to flatten a Parallel
> Append into a parent Append or MergeAppend node, but it seemed to me
> that there was a fair amount of duplication between
> accumulate_partialappend_subpath() and accumulate_append_subpaths, so
> what I did instead is folded all of the necessarily logic directly
> into accumulate_append_subpaths().  This approach also avoids some
> 

Re: [HACKERS] Variable substitution in psql backtick expansion

[Fabien COELHO]

Hello Tom & Michaël,


I think this patch should be rejected.

+1 for rejection [...]


The noes have it!

Note that the motivation was really symmetric completion:

 fabien=# \echo :VERSION_NAME
 11devel
 fabien=# \echo :VERSION_NUM
 11
 fabien=# \echo :VERSION
 PostgreSQL 11devel on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu 
5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609, 64-bit
 fabien=# \echo :SERVER_VERSION_NAME
 10.1
 fabien=# \echo :SERVER_VERSION_NUM
 11

But

 fabien=# \echo :SERVER_VERSION
 :SERVER_VERSION

To get it into a variable the work around is really:

 fabien=# SELECT version() AS "SERVER_VERSION" \gset
 fabien=# \echo :SERVER_VERSION
 PostgreSQL 10.1 on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu 
5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609, 64-bit

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

Re: [HACKERS] Proposal: Improve bitmap costing for lossy pages

[Dilip Kumar]

On Sat, Nov 11, 2017 at 3:25 AM, Robert Haas  wrote:
> On Thu, Nov 9, 2017 at 3:55 AM, amul sul  wrote:
>> It took me a little while to understand this calculation.  You have moved 
>> this
>> code from tbm_create(), but I think you should move the following
>> comment as well:
>
> I made an adjustment that I hope will address your concern here, made
> a few other adjustments, and committed this.
>
Thanks, Robert.
-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


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

Re: [HACKERS] How to implement a SP-GiST index as a extension module?

[Connor Wolf]

Ok, I've managed to get my custom index working.

It's all on github here: https://github.com/fake-name/pg-spgist_hamming, if
anyone else needs a fuzzy-image searching system
that can integrate into postgresql..

It should be a pretty good basis for anyone else to use if they want to
implement a SP-GiST index too.

Thanks!

On Sun, Nov 5, 2017 at 8:10 PM, Connor Wolf  wrote:

> Never mind, it turns out the issue boiled down to me declaring the
> wrong prefixType in my config function.
>
> TL;DR - PEBKAC
>
> On Sun, Nov 5, 2017 at 1:09 AM, Connor Wolf  com> wrote:
>
>> Ok, I've got everything compiling and it installs properly, but I'm
>> running into problems that I think are either a side-effect of implementing
>> picksplit incorrectly (likely), or a bug in SP-GiST(?).
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> __memcpy_sse2_unaligned () at ../sysdeps/x86_64/multiarch/me
>> mcpy-sse2-unaligned.S:159
>> 159 ../sysdeps/x86_64/multiarch/memcpy-sse2-unaligned.S: No such
>> file or directory.
>> (gdb) bt
>> #0  __memcpy_sse2_unaligned () at ../sysdeps/x86_64/multiarch/me
>> mcpy-sse2-unaligned.S:159
>> #1  0x004ecd66 in memcpy (__len=16, __src=,
>> __dest=0x13c9dd8) at /usr/include/x86_64-linux-gnu/bits/string3.h:53
>> #2  memcpyDatum (target=target@entry=0x13c9dd8, att=att@entry=0x7fff327325f4,
>> datum=datum@entry=18445692987396472528) at spgutils.c:587
>> #3  0x004ee06b in spgFormInnerTuple (state=state@entry
>> =0x7fff327325e0, hasPrefix=, prefix=18445692987396472528,
>> nNodes=8,
>> nodes=nodes@entry=0x13bd340) at spgutils.c:741
>> #4  0x004f508b in doPickSplit (index=index@entry=0x7f2cf9de7f98,
>> state=state@entry=0x7fff327325e0, current=current@entry=0x7fff32732020,
>> parent=parent@entry=0x7fff32732040, 
>> newLeafTuple=newLeafTuple@entry=0x13b9f00,
>> level=level@entry=0, isNulls=0 '\000', isNew=0 '\000') at
>> spgdoinsert.c:913
>> #5  0x004f6976 in spgdoinsert (index=index@entry=0x7f2cf9de7f98,
>> state=state@entry=0x7fff327325e0, heapPtr=heapPtr@entry=0x12e672c,
>> datum=12598555199787281,
>> isnull=0 '\000') at spgdoinsert.c:2053
>> #6  0x004ee5cc in spgistBuildCallback (index=index@entry
>> =0x7f2cf9de7f98, htup=htup@entry=0x12e6728, values=values@entry
>> =0x7fff327321e0,
>> isnull=isnull@entry=0x7fff32732530 "", tupleIsAlive=tupleIsAlive@entry=1
>> '\001', state=state@entry=0x7fff327325e0) at spginsert.c:56
>> #7  0x00534e8d in IndexBuildHeapRangeScan
>> (heapRelation=heapRelation@entry=0x7f2cf9ddc6c8,
>> indexRelation=indexRelation@entry=0x7f2cf9de7f98,
>> indexInfo=indexInfo@entry=0x1390ad8, allow_sync=allow_sync@entry=1
>> '\001', anyvisible=anyvisible@entry=0 '\000',
>> start_blockno=start_blockno@entry=0,
>> numblocks=4294967295, callback=0x4ee573 ,
>> callback_state=0x7fff327325e0) at index.c:2609
>> #8  0x00534f52 in IndexBuildHeapScan
>> (heapRelation=heapRelation@entry=0x7f2cf9ddc6c8,
>> indexRelation=indexRelation@entry=0x7f2cf9de7f98,
>> indexInfo=indexInfo@entry=0x1390ad8, allow_sync=allow_sync@entry=1
>> '\001', callback=callback@entry=0x4ee573 ,
>> callback_state=callback_state@entry=0x7fff327325e0) at index.c:2182
>> #9  0x004eeb74 in spgbuild (heap=0x7f2cf9ddc6c8,
>> index=0x7f2cf9de7f98, indexInfo=0x1390ad8) at spginsert.c:140
>> #10 0x00535e55 in index_build 
>> (heapRelation=heapRelation@entry=0x7f2cf9ddc6c8,
>> indexRelation=indexRelation@entry=0x7f2cf9de7f98,
>> indexInfo=indexInfo@entry=0x1390ad8, isprimary=isprimary@entry=0
>> '\000', isreindex=isreindex@entry=0 '\000') at index.c:2043
>> #11 0x00536ee8 in index_create 
>> (heapRelation=heapRelation@entry=0x7f2cf9ddc6c8,
>> indexRelationName=indexRelationName@entry=0x12dd600 "int8idx_2",
>> indexRelationId=16416, indexRelationId@entry=0, relFileNode=0,
>> indexInfo=indexInfo@entry=0x1390ad8, indexColNames=indexColNames@en
>> try=0x1390f40,
>> accessMethodObjectId=4000, tableSpaceId=0,
>> collationObjectId=0x12e6b18, classObjectId=0x12e6b38, coloptions=0x12e6b58,
>> reloptions=0, isprimary=0 '\000',
>> isconstraint=0 '\000', deferrable=0 '\000', initdeferred=0 '\000',
>> allow_system_table_mods=0 '\000', skip_build=0 '\000', concurrent=0 '\000',
>> is_internal=0 '\000', if_not_exists=0 '\000') at index.c:1116
>> #12 0x005d8fe6 in DefineIndex (relationId=relationId@entry=16413,
>> stmt=stmt@entry=0x12dd568, indexRelationId=indexRelationId@entry=0,
>> is_alter_table=is_alter_table@entry=0 '\000',
>> check_rights=check_rights@entry=1 '\001', check_not_in_use=check_not_in_
>> use@entry=1 '\001', skip_build=0 '\000',
>> quiet=0 '\000') at indexcmds.c:667
>> #13 0x00782057 in ProcessUtilitySlow (pstate=pstate@entry
>> =0x12dd450, pstmt=pstmt@entry=0x12db108,
>> queryString=queryString@entry=0x12da0a0 "CREATE INDEX int8idx_2 ON
>> int8tmp_2 USING spgist ( a vptree_ops );", 

Re: [HACKERS] Incorrect comment for build_child_join_rel

[Etsuro Fujita]

(2017/11/11 0:58), Robert Haas wrote:

On Fri, Nov 10, 2017 at 4:34 AM, Etsuro Fujita
  wrote:

Here is a small patch for $Subject.


Good catch.  Committed.


Thanks!

Best regards,
Etsuro Fujita


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

Re: [HACKERS] Variable substitution in psql backtick expansion

[Michael Paquier]

On Mon, Nov 13, 2017 at 5:21 AM, Tom Lane  wrote:
> Pavel Stehule  writes:
>> [ psql-server-version-2.patch ]
>
> I think this patch should be rejected.  It adds no new functionality;
> you can get the string in question with "select version()".  Moreover,
> you've been able to do that for lo these many years.  Any application
> that tried to depend on this new way of getting the string would fail
> when working with an older server or older psql.  That does not seem
> like a good property for a version check.  Also, because the string
> isn't especially machine-friendly, it's not very clear to me what the
> use-case is for an application to use it at all, rather than the other
> version formats we already provide.

+1 for rejection as version() returns PG_VERSION_STR already. It is
also already possible to define a VERSION variable psqlrc simply with
that:
\set VERSION 'version();'

+-- check consistency of SERVER_VERSION
+-- which is transmitted as GUC "server_version_raw"
+SELECT :'SERVER_VERSION' = VERSION()
+   AND :'SERVER_VERSION' = current_setting('server_version_raw')
+   AND :'SERVER_VERSION' = :'VERSION'
+   AS "SERVER_VERSION is consistent";
Not much enthusiastic with this test when thinking about cross-upgrades.
-- 
Michael


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

Re: [HACKERS] PATCH: psql tab completion for SELECT

[Michael Paquier]

On Mon, Nov 13, 2017 at 5:13 AM, David Fetter  wrote:
> Please add this to the upcoming (2018-01) commitfest at
> https://commitfest.postgresql.org/

You may want to scan the following thread as well:
https://www.postgresql.org/message-id/1328820579.11241.4.camel%40vanquo.pezone.net
And also you should be careful about things like WITH clauses...
-- 
Michael


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

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Andreas Karlsson]

On 11/10/2017 01:47 AM, Mark Rofail wrote:

I am sorry for the late reply


There is no reason for you to be. It did not take you 6 weeks to do a 
review. :) Thanks for this new version.



== Functional review

 >1) MATCH FULL does not seem to care about NULLS in arrays. In the
example below I expected both inserts into the referring table to fail.


It seems in your example the only failed case was: INSERT INTO fk VALUES 
(NULL, '{1}');

which shouldn't work, can you clarify this?


I think that if you use MATH FULL the query should fail if you have a 
NULL in the array.



 >2) To me it was not obvious that ON DELETE CASCADE would delete
the whole rows rather than delete the members from the array, and
this kind of misunderstanding can lead to pretty bad surprises in
production. I am leaning towards not supporting CASCADE.


I would say so too, maybe we should remove ON DELETE CASCADE until we 
have supported all remaining actions.


I am leaning towards this too. I would personally be fine with a first 
version without support for CASCADE since it is not obvious to me what 
CASCADE should do.



== The @>> operator
I would argue that allocating an array of datums and building an array 
would have the same complexity


I am not sure what you mean here. Just because something has the same 
complexity does not mean there can't be major performance differences.



== Code review

 >I think the code in RI_Initial_Check() would be cleaner if you
used "CROSS JOIN LATERAL unnest(col)" rather than having unnest() in
the target list. This way you would not need to rename all columns
and the code paths for the array case could look more like the code
path for the normal case.

Can you clarify what you mean a bit more?


I think the code would look cleaner if you generate the following query:

SELECT fk.x, fk.ys FROM ONLY t2 fk CROSS JOIN LATERAL 
pg_catalog.unnest(ys) a2 (v) LEFT OUTER JOIN ONLY t1 pk ON pk.x = fk.x 
AND pk.y = a2.v WHERE [...]


rather than:

SELECT fk.k1, fk.ak2 FROM (SELECT x k1, pg_catalog.unnest(ys) k2, ys ak2 
FROM ONLY t2) fk LEFT OUTER JOIN ONLY t1 pk ON pk.x = fk.k1 AND pk.y = 
fk.k2 WHERE [...]


= New stuff

When applying the patch I got some white space warnings:

Array-ELEMENT-foreign-key-v5.3.patch:1343: space before tab in indent, 
indent with spaces.

format_type_be(oprleft), 
format_type_be(oprright;
Array-ELEMENT-foreign-key-v5.3.patch:1345: trailing whitespace.

When compiling I got an error:

ri_triggers.c: In function ‘ri_GenerateQual’:
ri_triggers.c:2693:19: error: unknown type name ‘d’
   Oid   oprcommon;d
   ^
ri_triggers.c:2700:3: error: conflicting types for ‘oprright’
   oprright = get_array_type(operform->oprleft);
   ^~~~
ri_triggers.c:2691:9: note: previous declaration of ‘oprright’ was here
   Oid   oprright;
 ^~~~
: recipe for target 'ri_triggers.o' failed

When building the documentation I got two warnings:

/usr/bin/osx:catalogs.sgml:2349:17:W: empty end-tag
/usr/bin/osx:catalogs.sgml:2350:17:W: empty end-tag

When running the tests I got a failure in element_foreign_key.

Andreas


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

Re: [HACKERS] [bug fix] postgres.exe crashes with access violation on Windows while starting up

[Michael Paquier]

On Mon, Nov 13, 2017 at 7:37 AM, Noah Misch  wrote:
> On Fri, Nov 03, 2017 at 11:10:14AM +, Michael Paquier wrote:
>> I am
>> switching the patch as ready for committer, I definitely agree that
>> you are taking the write approach here.

s/write/right/.

> Committed both patches.

Thanks for double-checking, Noah.
-- 
Michael


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

Re: [HACKERS] [bug fix] postgres.exe crashes with access violation on Windows while starting up

[Noah Misch]

On Sat, Oct 28, 2017 at 03:43:02PM -0700, Michael Paquier wrote:
> couldn't we envisage to just use
> malloc? pgwin32_message_to_UTF16() is called in two places in elog.c,
> and there is a full control on the error code paths.

Switching to malloc is feasible, but it wouldn't enable PostgreSQL to handle
non-ASCII messages any earlier.  Messages should be ASCII-only until the
init_locale(LC_CTYPE) call initializes MessageEncoding.  (Before that call,
pgwin32_message_to_UTF16() assumes the message is UTF8-encoded.  I've expanded
the comments slightly.  We easily comply with that restriction today.)

On Fri, Nov 03, 2017 at 11:10:14AM +, Michael Paquier wrote:
> I am
> switching the patch as ready for committer, I definitely agree that
> you are taking the write approach here.

Committed both patches.


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

Re: [HACKERS] LDAPS

[Thomas Munro]

On Sat, Nov 4, 2017 at 2:05 AM, Thomas Munro
 wrote:
> I've only tested the attached lightly on FreeBSD + OpenLDAP and
> don't know if it'll work elsewhere.

While rebasing this on top of a nearby changes, I looked into how
portable it is.  The previous version unconditionally used
ldap_initialize() instead of ldap_init() in order to be able to pass
in ldap or ldaps.  According to the man pages on my system:

   At this time, ldap_open() and ldap_init() are deprecated in favor of
   ldap_initialize(), essentially because the latter allows to specify a
   schema in the URI and it explicitly returns an error code.

But:

1.  It looks like ldap_initialize() arrived in OpenLDAP 2.4 (2007),
which means that it won't work with RHEL5's OpenLDAP 2.3.  That's a
vintage still found in the build farm.  This new version of the patch
has a configure test so it can fall back to ldap_init(), dropping
ldaps support.  That is possibly also necessary for other
implementations.

2.  Windows doesn't have ldap_initialize(), but it has
ldap_sslinit()[1] which adds an SSL boolean argument.  I've included
(but not tested) code for that.  I would need a Windows + LDAP savvy
person to help test that.  I'm not sure if it should also do an
LDAP_OPT_SSL check to see if the server forced the connection back to
plaintext as shown in the Microsoft docs[2], or if that should be
considered OK, or it should be an option.

BTW, Stephen Layland posted a patch for ldaps years ago[3].  It must
have worked some other way though, because he mentions RHEL 4 and
OpenLDAP 2.2/2.3.  Unfortunately the patch wasn't attached and the
referenced webserver has disappeared from the intertubes.

I've added this to the January Commitfest.

[1] https://msdn.microsoft.com/en-us/library/aa366996(v=vs.85).aspx
[2] https://msdn.microsoft.com/en-us/library/aa366105(v=vs.85).aspx
[3] https://www.postgresql.org/message-id/20080426010240.gs5...@68k.org

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


ldaps-v3.patch
Description: Binary data

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

Re: [HACKERS] Variable substitution in psql backtick expansion

[Tom Lane]

Pavel Stehule  writes:
> [ psql-server-version-2.patch ]

I think this patch should be rejected.  It adds no new functionality;
you can get the string in question with "select version()".  Moreover,
you've been able to do that for lo these many years.  Any application
that tried to depend on this new way of getting the string would fail
when working with an older server or older psql.  That does not seem
like a good property for a version check.  Also, because the string
isn't especially machine-friendly, it's not very clear to me what the
use-case is for an application to use it at all, rather than the other
version formats we already provide.

regards, tom lane


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

Re: [HACKERS] PATCH: psql tab completion for SELECT

[David Fetter]

On Mon, Nov 06, 2017 at 05:28:55PM +1300, Edmund Horner wrote:
> Hi pgsql-hackers,
> 
> Here's a little draft patch to add *some* tab completion ability for
> SELECT in psql.  I have often missed the ability, especially with
> invocations of utility functions.
> 
> It would be nice to be able to suggest column names from the
> relevant tables in the query, but, as the SQL language puts the
> column list before the FROM clause, we have to use columns from all
> tables; I'm certainly not the first to be frustrated by this
> language feature, but we can't do much about it.
> 
> What my patch does:
> 
> For a command line with the single word SELECT, column names and
> function names will be used for completions of the next word.
> Function names have a "(" suffix added, largely because it makes
> them easier to distinguish in the completion list.
> 
> Only the first select-list item can be tab-completed; i.e. SELECT
> foo, b won't find column bar.

That can be fixed later.

> Examples:
> 
> postgres=# select rel
> relacl   relchecksrelhasindex
> relhassubclass   (etc.)
> 
> postgres=# select str
> string_to_array(  strip(strpos(

Neat!

Please add this to the upcoming (2018-01) commitfest at
https://commitfest.postgresql.org/

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


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

Re: [HACKERS] Row Level Security Bug ?

[Joe Conway]

On 11/12/2017 10:17 AM, Andrea Adami wrote:
> if i do:
> 
> SET ROLE 'manage...@scuola-1.it '

[SELECT from table]

> i see only one row (as expected)
> 
> but when i do:

[SELECT from VIEWs]

> I see all the rows always
> 
> this way i lack all the row level security i defined
> 
> is this either a bug or it's made by design ?
> if it's made by design why ?
> Is there  a way to write view that respect the row level security ?
> For my point of view is a nonsense make a row level security that
> doesn't work with the view.

See:
https://www.postgresql.org/docs/10/static/sql-createview.html
In particular: "Access to tables referenced in the view is determined by
permissions of the view owner."

And:
https://www.postgresql.org/docs/10/static/ddl-rowsecurity.html
"Superusers and roles with the BYPASSRLS attribute always bypass the row
security system when accessing a table. Table owners normally bypass row
security as well, though a table owner can choose to be subject to row
security with ALTER TABLE ... FORCE ROW LEVEL SECURITY."

HTH,

Joe

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



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Fix number skipping in to_number

[Tom Lane]

Oliver Ford  writes:
> [ 0001-apply-number-v3.patch ]

I looked at this patch briefly and have a couple of comments:

* It seems entirely wrong to be matching to L_thousands_sep in the
NUM_COMMA case; that format code is by definition not locale aware,
so it should be matching to plain ',' independently of locale.

* Don't we need to fix the NUM_L (currency symbol) case in the
same manner?  (The NUM_D and NUM_S cases are handled in
NUM_numpart_from_char and seem ok at a quick glance.)

* I'm not in love with the noadd flag.  Other places in this
switch that want to skip the final increment do it with
"continue", and I think this should do likewise.

regards, tom lane


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

Re: [HACKERS] New gist vacuum.

[Andrey Borodin]

Hello!

> 31 янв. 2016 г., в 17:18, Alvaro Herrera  
> написал(а):
> 
> Костя Кузнецов wrote:
>> Thank you, Jeff.I reworking patch now. All // warning will 
>> be deleted.About memory consumption new version will control size 
>> of stack and will operate with map of little size because i want delete old 
>> style vacuum(now if maintenance_work_mem less than needed to build info map 
>> we use old-style vacuum with logical order).
> 
> You never got around to submitting the updated version of this patch,
> and it's been a long time now, so I'm marking it as returned with
> feedback for now.  Please do submit a new version once you have it,
> since this seems to be very useful.

I've rebased patch (see attachment), it seems to work. It still requires a bit 
of styling, docs and tests, at least.
Also, I thinks that hash table is not very good option if we have all pages 
there: we should either use array or do not fill table for every page.

If author and community do not object, I want to continue work on Konstantin's 
patch.

Best regards, Andrey Borodin.


0001-GiST-VACUUM-rebase.patch
Description: Binary data

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

Re: [HACKERS] PSA: don't be in a hurry to update to XCode 9.0

[Tom Lane]

Dave Cramer  writes:
> Did you ever find a solution to this without updating ?

No.  I filed a bug report which Apple seems uninterested in, perhaps
not too surprisingly.

regards, tom lane


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

Re: [HACKERS] PSA: don't be in a hurry to update to XCode 9.0

[Dave Cramer]

Tom,

Did you ever find a solution to this without updating ?

Dave Cramer

da...@postgresintl.com
www.postgresintl.com

On 21 September 2017 at 13:01, Dave Cramer  wrote:

> Too late I just stumbled over this as well!
>
> Dave Cramer
>
> da...@postgresintl.com
> www.postgresintl.com
>
> On 20 September 2017 at 14:34, Tom Lane  wrote:
>
>> It seems to install some libraries that depend on
>> /usr/lib/system/libsystem_darwin.dylib, which doesn't exist.
>> Googling suggests it will be there in macOS 10.13,
>> but on Sierra or earlier, you're outta luck.
>>
>> It looks like the only directly-affected PG dependency is
>> libxml; if you leave out --with-libxml you can still build.
>>
>> I found this out the hard way on longfin's host, so I've
>> temporarily removed --with-libxml from that animal's
>> configuration to restore it to service.  I trust I'll be
>> able to re-enable that after 10.13 comes out.
>>
>> regards, tom lane
>>
>>
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>>
>
>

Re: [HACKERS] WIP: Covering + unique indexes.

[Andrey Borodin]

Hi!
+1 for pushing this. I'm really looking forward to see this in 11.

> 31 окт. 2017 г., в 13:21, Anastasia Lubennikova 
>  написал(а):
> 
> Updated version is attached. It applies to the commit 
> e4fbf22831c2bbcf032ee60a327b871d2364b3f5.
> The first patch patch contains changes in general index routines
> and the second one contains btree specific changes.
> 
> This version contains fixes of the issues mentioned in the thread above and 
> passes all existing tests.
> But still it requires review and testing, because the merge was quite uneasy.
> Especially I worry about integration with partitioning. I'll add some more 
> tests in the next message.

I've been doing benchmark tests a year ago, so I skip this part in this review.

I've done some stress tests with pgbench, replication etc. Everything was fine 
until I plugged in amcheck.
If I create cluster with this [0] and then 
./pgbench -i -s 50

create index on pgbench_accounts (abalance) include (bid,aid,filler); 
create extension amcheck;

--and finally 
SELECT bt_index_check(c.oid), c.relname, c.relpages
FROM pg_index i
JOIN pg_opclass op ON i.indclass[0] = op.oid
JOIN pg_am am ON op.opcmethod = am.oid
JOIN pg_class c ON i.indexrelid = c.oid
JOIN pg_namespace n ON c.relnamespace = n.oid
WHERE am.amname = 'btree' AND n.nspname = 'public'
AND c.relpersistence != 't'
AND i.indisready AND i.indisvalid
ORDER BY c.relpages DESC LIMIT 100;
--just copypasted from amcheck docs with minor corrections

Postgres crashes: 
TRAP: FailedAssertion("!(((const void*)() != ((void*)0)) && 
(scankey->sk_attno) > 0)", File: "nbtsearch.c", Line: 466)

May be I'm doing something wrong or amcheck support will go with different 
patch?

Few minor nitpicks:
0. PgAdmin fails to understand what is going on [1] . It is clearly problem of 
PgAdmin, pg_dump works as expected.
1. ISTM index_truncate_tuple() can be optimized. We only need to reset tuple 
length and infomask. But this should not be hotpath, anyway, so I propose 
ignoring this for current version.
2. I've done grammarly checking :) This comma seems redundant [2]
I don't think something of these items require fixing.

Thanks for working on this, I believe it is important.

Best regards, Andrey Borodin.

[0] https://github.com/x4m/pgscripts/blob/master/install.sh
[1] https://yadi.sk/i/ro9YKFqo3PcwFT
[2] 
https://github.com/x4m/postgres_g/commit/657c28952d923d8c150e6cabb3bdcbbc44a641b6?diff=unified#diff-640baf2937029728a8d51cccd554c2eeR1291

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

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

[Thomas Munro]

On Sun, Nov 12, 2017 at 8:51 PM, Amit Kapila  wrote:
> On Sun, Nov 12, 2017 at 9:18 AM, Thomas Munro
>  wrote:
>> How about parallel_leader_participation = on|off?  The attached
>> version has it that way, and adds regression tests to exercise on, off
>> and off-but-couldn't-start-any-workers for both kinds of gather node.
>>
>> I'm not sure why node->need_to_rescan is initialised by both
>> ExecGatherInit() and ExecGather().  Only the latter's value matters,
>> right?
>>
>
> I don't see anything like need_to_rescan in the GatherState node.  Do
> you intend to say need_to_scan_locally?  If yes, then I think whatever
> you said is right.

Right, that's what I meant to write.  Thanks.

>> I've added this to the January Commitfest.
>>
>
> +1 to this idea.  Do you think such an option at table level can be
> meaningful?  We have a parallel_workers as a storage option for
> tables, so users might want leader to participate in parallelism only
> for some of the tables.

I'm not sure.  I think the reason for turning it off (other than
developer testing) would be that the leader is getting tied up doing
work that takes a long time (sorting, hashing, aggregating) and that's
causing the workers to be blocked because their output queue is full.
I think that type of behaviour comes from certain plan types, and it
probably wouldn't make sense to associate this behaviour with the
tables you're scanning.

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


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

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

[Amit Kapila]

On Sun, Nov 12, 2017 at 9:18 AM, Thomas Munro
 wrote:
> On Sat, Oct 21, 2017 at 8:09 AM, Robert Haas  wrote:
>> On Tue, Oct 17, 2017 at 7:27 AM, Thomas Munro
>>  wrote:
>>
>> I don't think overloading force_parallel_mode is a good idea, but
>> having some other GUC for this seems OK to me.  Not sure I like
>> multiplex_gather, though.
>
> How about parallel_leader_participation = on|off?  The attached
> version has it that way, and adds regression tests to exercise on, off
> and off-but-couldn't-start-any-workers for both kinds of gather node.
>
> I'm not sure why node->need_to_rescan is initialised by both
> ExecGatherInit() and ExecGather().  Only the latter's value matters,
> right?
>

I don't see anything like need_to_rescan in the GatherState node.  Do
you intend to say need_to_scan_locally?  If yes, then I think whatever
you said is right.


> I've added this to the January Commitfest.
>

+1 to this idea.  Do you think such an option at table level can be
meaningful?  We have a parallel_workers as a storage option for
tables, so users might want leader to participate in parallelism only
for some of the tables.


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


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

Re: [HACKERS] GatherMerge misses to push target list

[Amit Kapila]

On Sat, Nov 11, 2017 at 5:05 PM, Robert Haas  wrote:
> On Mon, Sep 18, 2017 at 7:02 AM, Rushabh Lathia
>  wrote:
>>> In that case, can you please mark the patch [1] as ready for committer in
>>> CF app
>>
>> Done.
>
> I think this patch is mostly correct, but I think the change to
> planner.c isn't quite right.  ordered_rel->reltarget is just a dummy
> target list that produces nothing.  Instead, I think we should pass
> path->pathtarget, representing the idea that whatever Gather Merge
> produces as output is the same as what you put into it.
>

Agreed.  Your change looks good to me.

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


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

Re: [HACKERS] possible encoding issues with libxml2 functions

[Pavel Stehule]

2017-11-11 21:19 GMT+01:00 Noah Misch :

> On Sun, Nov 05, 2017 at 06:10:04PM +0100, Pavel Stehule wrote:
> > Hi
> >
> > 2017-11-05 4:07 GMT+01:00 Noah Misch :
> >
> > > On Tue, Oct 17, 2017 at 06:06:40AM +0200, Pavel Stehule wrote:
> > > > Please, if you can, try it write. I am little bit lost :)
> > >
> > > I'm attaching the patch I desired.  Please review.  This will probably
> miss
> > > this week's minor releases.  If there's significant support, I could
> > > instead
> > > push before the wrap.
> > >
> >
> > I have not any objection to this solution. It fixes my regress tests too.
> >
> > I checked it and it is working.
>
> Pushed, but the buildfarm shows I didn't get the test quite right for the
> non-xml, non-UTF8 case.  Fixing.
>

Thank you

Pavel

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

[Thomas Munro]

On Sat, Oct 21, 2017 at 8:09 AM, Robert Haas  wrote:
> On Tue, Oct 17, 2017 at 7:27 AM, Thomas Munro
>  wrote:
>> While testing parallelism work I've wanted to be able to prevent
>> gather nodes from running the plan in the leader process, and I've
>> heard others say the same.  One way would be to add a GUC
>> "multiplex_gather", like in the attached patch.  If you set it to off,
>> Gather and Gather Merge won't run the subplan unless they have to
>> because no workers could be launched.  I thought about adding a new
>> value for force_parallel_mode instead, but someone mentioned they
>> might want to do this on a production system too and
>> force_parallel_mode is not really for end users.  Better ideas?
>
> I don't think overloading force_parallel_mode is a good idea, but
> having some other GUC for this seems OK to me.  Not sure I like
> multiplex_gather, though.

How about parallel_leader_participation = on|off?  The attached
version has it that way, and adds regression tests to exercise on, off
and off-but-couldn't-start-any-workers for both kinds of gather node.

I'm not sure why node->need_to_rescan is initialised by both
ExecGatherInit() and ExecGather().  Only the latter's value matters,
right?

I've added this to the January Commitfest.

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


parallel-leader-participation-v1.patch
Description: Binary data

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

Re: [HACKERS] pgbench regression test failure

[Steve Singer]

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

This causes the pgbench tests to fail (consistently) with

not ok 194 - pgbench late throttling stdout /(?^:processed: [01]/10)/


When I run pgbench manually I get (-t 10 --rate=10 --latency-limit=1 -n -r)

number of transactions actually processed: 10/10
number of transactions skipped: 10 (100.000 %)

Prior to the patch I was getting.

number of transactions actually processed: 0/10
number of transactions skipped: 10 (100.000 %)



@@ -3539,7 +3542,7 @@ printResults(TState *threads, StatsData *total, 
instr_time total_time,^M
{^M
printf("number of transactions per client: %d\n", nxacts);^M
printf("number of transactions actually processed: " 
INT64_FORMAT "/%d\n",^M
-  total->cnt - total->skipped, nxacts * nclients);^M
+  total->cnt, nxacts * nclients);^M

I think you want ntx instead of total->cnt here. 




The new status of this patch is: Waiting on Author

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

Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks

[Mark Dilger]

> On Nov 10, 2017, at 3:58 PM, Stephen Frost  wrote:
> 
> Michael, Tom,
> 
> * Michael Paquier (michael.paqu...@gmail.com) wrote:
>> On Fri, Nov 10, 2017 at 10:00 AM, Tom Lane  wrote:
>>> Stephen Frost  writes:
 I'm guessing no, which essentially means that *we* consider access to
 lo_import/lo_export to be equivilant to superuser and therefore we're
 not going to implement anything to try and prevent the user who has
 access to those functions from becoming superuser.  If we aren't willing
 to do that, then how can we really say that there's some difference
 between access to these functions and being a superuser?
>>> 
>>> We seem to be talking past each other.  Yes, if a user has malicious
>>> intentions, it's possibly to parlay lo_export into obtaining a superuser
>>> login (I'm less sure that that's necessarily true for lo_import).
>>> That does NOT make it "equivalent", except perhaps in the view of someone
>>> who is only considering blocking malevolent actors.  It does not mean that
>>> there's no value in preventing a task that needs to run lo_export from
>>> being able to accidentally destroy any data in the database.  There's a
>>> range of situations where you are concerned about accidents and errors,
>>> not malicious intent; but your argument ignores those use-cases.
>> 
>> That will not sound much as a surprise as I spawned the original
>> thread, but like Robert I understand that getting rid of all superuser
>> checks is a goal that we are trying to reach to allow admins to have
>> more flexibility in handling permissions to a subset of objects.
>> Forcing an admin to give full superuser rights to one user willing to
>> work only on LOs import and export is a wrong concept.
> 
> The problem I have with this is that 'full superuser rights' actually
> are being granted by this.  You're able to read any file and write any
> file on the filesystem that the PG OS user can.  How is that not 'full
> superuser rights'?  The concerns about a mistake being made are just as
> serious as they are with someone who has superuser rights as someone
> could pretty easily end up overwriting the control file or various other
> extremely important bits of the system.  This isn't just about what
> 'blackhats' can do with this.
> 
> As I mentioned up-thread, if we want to make it so that non-superusers
> can use lo_import/lo_export, then we should do that in a way that
> allows the administrator to specify exactly the rights they really want
> to give, based on a use-case for them.  There's no use-case for allowing
> someone to use lo_export() to overwrite pg_control.  The use-case would
> be to allow them to export to a specific directory (or perhaps a set of
> directories), or to read from same.  The same is true of COPY.  Further,
> I wonder what would happen if we allow this and then someone comes along
> and re-proposes the 'CREATE DIRECTORY' concept that I had once proposed
> (ideally with things cleaned up and tightened up to avoid there being
> issues using it) to address the actual use-case for these functions to
> be available to a non-superuser.  We wouldn't be able to change these
> functions to start checking the 'directory' rights or we would break
> existing non-superuser usage of them.  I imagine we could create
> additional functions which check the 'directory' privileges, but that's
> hardly ideal, imv.
> 
> I'm disappointed to apparently be in the minority on this, but that
> seems to be the case unless someone else wishes to comment.  While I
> appreciate the discussion, I'm certainly no more convinced today than I
> was when I first saw this go in that allowing these functions to be
> granted to non-superusers does anything but effectively make them into
> superusers who aren't explicitly identified as superusers.

Just to help understand your concern, and not to propose actual features,
would it help if there were

(1) a function, perhaps pg_catalog.pg_exploiters(), which would return all
roles that have been granted exploitable privileges, such that it would be
easier to identify all superusers, including those whose superuserishness
derives from a known export, and

(2) a syntax change for GRANT that would require an extra token, so
that you'd have to write something like

GRANT EXPLOITABLE lo_export TO trusted_user_foo

so that you couldn't unknowingly grant a dangerous privilege.

Or is there more to it than that?

mark







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

Re: [HACKERS] Partition-wise aggregation/grouping

[Konstantin Knizhnik]

On 10/27/2017 02:01 PM, Jeevan Chalke wrote:

Hi,

Attached new patch-set here. Changes include:

1. Added separate patch for costing Append node as discussed up-front in the
patch-set.
2. Since we now cost Append node, we don't need partition_wise_agg_cost_factor
GUC. So removed that. The remaining patch hence merged into main implementation
patch.
3. Updated rows in test-cases so that we will get partition-wise plans.

Thanks


I applied partition-wise-agg-v6.tar.gz patch to  the master and use shard.sh 
example from https://www.postgresql.org/message-id/14577.1509723225%40localhost
Plan for count(*) is the following:

shard=# explain select count(*) from orders;
  QUERY PLAN
---
 Finalize Aggregate  (cost=100415.29..100415.30 rows=1 width=8)
   ->  Append  (cost=50207.63..100415.29 rows=2 width=8)
 ->  Partial Aggregate  (cost=50207.63..50207.64 rows=1 width=8)
   ->  Foreign Scan on orders_0 (cost=101.00..50195.13 rows=5000 
width=0)
 ->  Partial Aggregate  (cost=50207.63..50207.64 rows=1 width=8)
   ->  Foreign Scan on orders_1 (cost=101.00..50195.13 rows=5000 
width=0)


We really calculate partial aggregate for each partition, but to do we still 
have to fetch all data from remote host.
So for foreign partitions such plans is absolutely inefficient.
Amy be it should be combined with some other patch?
For example, with  agg_pushdown_v4.tgz patch 
https://www.postgresql.org/message-id/14577.1509723225%40localhost ?
But it is not applied after partition-wise-agg-v6.tar.gz patch.
Also postgres_fdw in 11dev is able to push down aggregates without 
agg_pushdown_v4.tgz patch.

In 0009-Teach-postgres_fdw-to-push-aggregates-for-child-rela.patch
there is the following check:

 /* Partial aggregates are not supported. */
+   if (extra->isPartial)
+   return;

If we just comment this line then produced plan will be the following:

shard=# explain select sum(product_id) from orders;
   QUERY PLAN

 Finalize Aggregate  (cost=308.41..308.42 rows=1 width=8)
   ->  Append  (cost=144.18..308.41 rows=2 width=8)
 ->  Foreign Scan  (cost=144.18..154.20 rows=1 width=8)
   Relations: Aggregate on (public.orders_0 orders)
 ->  Foreign Scan  (cost=144.18..154.20 rows=1 width=8)
   Relations: Aggregate on (public.orders_1 orders)
(6 rows)

And it is actually desired plan!
Obviously such approach will not always work. FDW really doesn't support 
partial aggregates now.
But for most frequently used aggregates: sum, min, max, count 
aggtype==aggtranstype and there is no difference
between partial and normal aggregate calculation.
So instead of (extra->isPartial) condition we can add more complex check which 
will traverse pathtarget expressions and
check if it can be evaluated in this way. Or... extend FDW API to support 
partial aggregation.

But even the last plan is not ideal: it will calculate predicates at each 
remote node sequentially.
There is parallel append patch:
https://www.postgresql.org/message-id/CAJ3gD9ctEcrVUmpY6fq_JUB6WDKGXAGd70EY68jVFA4kxMbKeQ%40mail.gmail.com
but ... FDW doesn't support parallel scan, so parallel append can not be 
applied in this case.
And we actually do not need parallel append with all its dynamic workers here.
We just need to start commands at all remote servers and only after it fetch 
results (which can be done sequentially).

I am investigating problem of efficient execution of OLAP queries on sharded 
tables (tables with remote partitions).
After reading all this threads and corresponding  patches, it seems to me
that we already have most of parts of the puzzle, what we need is to put them 
on right places and may be add missed ones.
I wonder if somebody is busy with it and can I somehow help here?

Also I am not quite sure about the best approach with parallel execution of 
distributed query at all nodes.
Should we make postgres_fdw parallel safe and use parallel append? How 
difficult it will be?
Or in addition to parallel append we should also have "asynchronous append" 
which will be able to initiate execution at all nodes?
It seems to be close to merge append, because it should simultaneously traverse 
all cursors.

Looks like second approach is easier for implementation. But in case of sharded 
table, distributed query may need to traverse both remote
and local shards and this approach doesn't allow to processed several local 
shards in parallel.

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



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

Re: [HACKERS] possible encoding issues with libxml2 functions

[Noah Misch]

On Sun, Nov 05, 2017 at 06:10:04PM +0100, Pavel Stehule wrote:
> Hi
> 
> 2017-11-05 4:07 GMT+01:00 Noah Misch :
> 
> > On Tue, Oct 17, 2017 at 06:06:40AM +0200, Pavel Stehule wrote:
> > > Please, if you can, try it write. I am little bit lost :)
> >
> > I'm attaching the patch I desired.  Please review.  This will probably miss
> > this week's minor releases.  If there's significant support, I could
> > instead
> > push before the wrap.
> >
> 
> I have not any objection to this solution. It fixes my regress tests too.
> 
> I checked it and it is working.

Pushed, but the buildfarm shows I didn't get the test quite right for the
non-xml, non-UTF8 case.  Fixing.


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

Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks

[Tom Lane]

Stephen Frost  writes:
> * Michael Paquier (michael.paqu...@gmail.com) wrote:
>> Forcing an admin to give full superuser rights to one user willing to
>> work only on LOs import and export is a wrong concept.

> The problem I have with this is that 'full superuser rights' actually
> are being granted by this.  You're able to read any file and write any
> file on the filesystem that the PG OS user can.  How is that not 'full
> superuser rights'?

It doesn't cause, say, "DELETE FROM pg_proc;" to succeed.

You're still not getting the point that this is for people who want
self-imposed restrictions.  Sure, you can't give out lo_export privilege
to someone you would not trust with superuser.  But somebody who needs
lo_export, and is trustworthy enough to have it, may still wish to do
the inside-the-database part of their work with less than full superuser,
just as a safety measure.  It's the *exact same* reason why cautious
people use "sudo" rather than just running as root all the time.

> As I mentioned up-thread, if we want to make it so that non-superusers
> can use lo_import/lo_export, then we should do that in a way that
> allows the administrator to specify exactly the rights they really want
> to give, based on a use-case for them.

Our current answer for that is wrapper functions.  This patch does not
make that answer any less applicable than before.

> I wonder what would happen if we allow this and then someone comes along
> and re-proposes the 'CREATE DIRECTORY' concept that I had once proposed
> (ideally with things cleaned up and tightened up to avoid there being
> issues using it) to address the actual use-case for these functions to
> be available to a non-superuser.  We wouldn't be able to change these
> functions to start checking the 'directory' rights or we would break
> existing non-superuser usage of them.

This is a straw man argument --- if we tighten up the function to check
this as-yet-nonexistent feature, how is that not breaking existing
use-cases anyway?

regards, tom lane


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

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Sat, Nov 11, 2017 at 6:48 AM, Michael Paquier 
wrote:
>
> On Sat, Nov 11, 2017 at 12:49 AM, Fabrízio de Royes Mello
>  wrote:
> > New version attached.
>
> Thanks.
>
> +++ b/src/test/modules/Makefile
>   test_extensions \
> + test_session_hooks \
>   test_parser
> Better if that's in alphabetical order.
>

Fixed.


> That's a nit though, so I am switching the patch as ready for committer.
>

Thank you so much for your review.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 05c5c19..d3156ad 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -169,6 +169,9 @@ static ProcSignalReason RecoveryConflictReason;
 static MemoryContext row_description_context = NULL;
 static StringInfoData row_description_buf;
 
+/* Hook for plugins to get control at start of session */
+session_start_hook_type session_start_hook = NULL;
+
 /* 
  *		decls for routines only used in this file
  * 
@@ -3857,6 +3860,9 @@ PostgresMain(int argc, char *argv[],
 	if (!IsUnderPostmaster)
 		PgStartTime = GetCurrentTimestamp();
 
+	if (session_start_hook)
+		(*session_start_hook) ();
+
 	/*
 	 * POSTGRES main processing loop begins here
 	 *
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 20f1d27..16ec376 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -76,6 +76,8 @@ static bool ThereIsAtLeastOneRole(void);
 static void process_startup_options(Port *port, bool am_superuser);
 static void process_settings(Oid databaseid, Oid roleid);
 
+/* Hook for plugins to get control at end of session */
+session_end_hook_type session_end_hook = NULL;
 
 /*** InitPostgres support ***/
 
@@ -1154,6 +1156,10 @@ ShutdownPostgres(int code, Datum arg)
 	 * them explicitly.
 	 */
 	LockReleaseAll(USER_LOCKMETHOD, true);
+
+	/* Hook at session end */
+	if (session_end_hook)
+		(*session_end_hook) ();
 }
 
 
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index f8c535c..9f05bfb 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -35,6 +35,13 @@ extern PGDLLIMPORT const char *debug_query_string;
 extern int	max_stack_depth;
 extern int	PostAuthDelay;
 
+/* Hook for plugins to get control at start and end of session */
+typedef void (*session_start_hook_type) (void);
+typedef void (*session_end_hook_type) (void);
+
+extern PGDLLIMPORT session_start_hook_type session_start_hook;
+extern PGDLLIMPORT session_end_hook_type session_end_hook;
+
 /* GUC-configurable parameters */
 
 typedef enum
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index b7ed0af..7246552 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -15,6 +15,7 @@ SUBDIRS = \
 		  test_pg_dump \
 		  test_rbtree \
 		  test_rls_hooks \
+		  test_session_hooks \
 		  test_shm_mq \
 		  worker_spi
 
diff --git a/src/test/modules/test_session_hooks/.gitignore b/src/test/modules/test_session_hooks/.gitignore
new file mode 100644
index 000..5dcb3ff
--- /dev/null
+++ b/src/test/modules/test_session_hooks/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/test_session_hooks/Makefile b/src/test/modules/test_session_hooks/Makefile
new file mode 100644
index 000..c5c3860
--- /dev/null
+++ b/src/test/modules/test_session_hooks/Makefile
@@ -0,0 +1,21 @@
+# src/test/modules/test_session_hooks/Makefile
+
+MODULES = test_session_hooks
+PGFILEDESC = "test_session_hooks - Test session hooks with an extension"
+
+EXTENSION = test_session_hooks
+DATA = test_session_hooks--1.0.sql
+
+REGRESS = test_session_hooks
+REGRESS_OPTS = --temp-config=$(top_srcdir)/src/test/modules/test_session_hooks/session_hooks.conf
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/test_session_hooks
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/test_session_hooks/README b/src/test/modules/test_session_hooks/README
new file mode 100644
index 000..9cb4202
--- /dev/null
+++ b/src/test/modules/test_session_hooks/README
@@ -0,0 +1,2 @@
+test_session_hooks is an example of how to use session start and end
+hooks.
diff --git a/src/test/modules/test_session_hooks/expected/test_session_hooks.out b/src/test/modules/test_session_hooks/expected/test_session_hooks.out
new file mode 

Re: [HACKERS] parallelize queries containing initplans

[Amit Kapila]

On Sat, Nov 11, 2017 at 12:15 AM, Robert Haas  wrote:
> On Tue, Nov 7, 2017 at 4:45 AM, Amit Kapila  wrote:
>> As mentioned, changed the status of the patch in CF app.
>
> I spent some time reviewing this patch today and found myself still
> quite uncomfortable with the fact that it was adding execution-time
> work to track the types of parameters - types that would usually not
> even be used.  I found the changes to nodeNestLoop.c to be
> particularly objectionable, because we could end up doing the work
> over and over when it is actually not needed at all, or at most once.
>

That's right, but we are just accessing tuple descriptor to get the
type, there shouldn't be much work involved in that.  However, I think
your approach has a merit that we don't need to even do that during
execution time.

> I decided to try instead teaching the planner to keep track of the
> types of PARAM_EXEC parameters as they were created, and that seems to
> work fine.  See 0001, attached.
>

This looks good to me.

> 0002, attached, is my worked-over version of the rest of the patch.  I
> moved the code that serializes and deserializes PARAM_EXEC from
> nodeSubplan.c -- which seemed like a strange choice - to
> execParallel.c.
>

I have tried to follow the practice we have used for param extern
params (SerializeParamList is in params.c) and most of the handling of
initplans is done in nodeSubplan.c, so I choose to keep the newly
added functions there.  However, I think keeping it in execParallel.c
is also okay as we do it for serialize plan.

>  I removed the type OID from the serialization format
> because there's no reason to do that any more; the worker already
> knows the types from the plan.  I did some renaming of the functions
> involved and some adjustment of the comments to refer to "PARAM_EXEC
> parameters" instead of initPlan parameters, because there's no reason
> that I can see why this can only work for initPlans.  A Gather node on
> the inner side of a nested loop doesn't sound like a great idea, but I
> think this infrastructure could handle it (though it would need some
> more planner work).
>

I think it would need some work in execution as well because the size
won't be fixed in that case for varchar type of params.  We might end
up with something different as well.

>  I broke a lot of long lines in your version of
> the patch into multiple lines; please try to be attentive to this
> issue when writing patches in general, as it is a bit tedious to go
> through and insert line breaks in many places.
>

Okay, but I sometimes rely on pgindent for such things as for few
things it becomes difficult to decide which way it will be better.

> Please let me know your thoughts on the attached patches.
>

Few minor comments:
1.
--- a/src/backend/executor/execParallel.c
+++ b/src/backend/executor/execParallel.c
@@ -23,6 +23,7 @@

 #include "postgres.h"

+#include "executor/execExpr.h"
 #include "executor/execParallel.h"
 #include "executor/executor.h"
 #include "executor/nodeBitmapHeapscan.h"
@@ -31,6 +32,7 @@
 #include "executor/nodeIndexscan.h"
 #include "executor/nodeIndexonlyscan.h"
 #include "executor/nodeSeqscan.h"
+#include "executor/nodeSubplan.h"

This is not required if we move serialize and other functions to execParallel.c

2.
+set_param_references(PlannerInfo *root, Plan *plan)
+{
+ Assert(IsA(plan, Gather) ||IsA(plan, GatherMerge));

I think there should be a space after || operator.

3.
+/*
+ * Serialize ParamExecData params corresponding to initplans.
+ *
..
+/*
+ * Restore ParamExecData params corresponding to initplans.
+ */

Shouldn't we change the reference to initplans here as well?

I have fixed the first two in attached patch and left the last one as
I was not sure what you have in mind

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


0002-pq-pushdown-initplan-rebased-1.patch
Description: Binary data

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

Re: [HACKERS] [PROPOSAL] Temporal query processing with range types

[Robert Haas]

On Fri, Oct 6, 2017 at 3:22 PM, Mike Rylander  wrote:
> I've also been following this feature with great interest, and would
> definitely throw whatever tiny weight I have, sitting out here in the
> the peanut gallery, behind accepting the ALIGN and NORMALIZE syntax.
> I estimate that about a third of the non-trivial queries in the
> primary project I work on (and have, on Postgres, for the last 13+
> years) would be simpler with support of the proposed syntax, and some
> of the most complex business logic would be simplified nearly to the
> point of triviality.

This is really good input.  If the feature weren't useful, then it
wouldn't make sense to try to figure out how to integrate it, but if
it is, then we should try.

I don't think that implementing a feature like this by SQL
transformation can work.  It's certainly got the advantage of
simplicity of implemention, but there are quite a few things that seem
like they won't always work correctly.  For instance:

+ * INPUT:
+ * (r ALIGN s ON q WITH (r.t, s.t)) c
+ *
+ *  where r and s are input relations, q can be any
+ *  join qualifier, and r.t, s.t can be any column name. The latter
+ *  represent the valid time intervals, that is time point start,
+ *  and time point end of each tuple for each input relation. These
+ *  are two half-open, i.e., [), range typed values.
+ *
+ * OUTPUT:
+ *  (
+ * SELECT r.*, GREATEST(LOWER(r.t), LOWER(s.t)) P1,
+ * LEAST(UPPER(r.t), UPPER(s.t)) P2
+ *  FROM
+ *  (
+ *  SELECT *, row_id() OVER () rn FROM r
+ *  ) r
+ *  LEFT OUTER JOIN
+ *  s
+ *  ON q AND r.t && s.t
+ *  ORDER BY rn, P1, P2
+ *  ) c

One problem with this is that we end up looking up functions in
pg_catalog by name: LOWER, UPPER, LEAST, GREATEST.  In particular,
when we do this...

+fcUpperRarg = makeFuncCall(SystemFuncName("upper"),
+   list_make1(crRargTs),
+   UNKNOWN_LOCATION);

...we're hoping and praying that we're going to latch onto the first of these:

rhaas=# \df upper
  List of functions
   Schema   | Name  | Result data type | Argument data types |  Type
+---+--+-+
 pg_catalog | upper | anyelement   | anyrange| normal
 pg_catalog | upper | text | text| normal
(2 rows)

But that's only true as long as there isn't another function in
pg_catalog with a match to the specific range type that is being used
here, and there's nothing to stop a user from creating one, and then
their query, which does not anywhere in its query text mention the
name of that function, will start failing.  We're not going to accept
that limitation.  Looking up functions by name rather than by OID or
using an opclass or something is pretty much a death sentence for a
core feature, and this patch does a lot of it.

A related problem is that, because all of this transformation is being
done in the parser, when you use this temporal syntax to create a
view, and then run pg_dump to dump that view, you are going to (I
think) get the transformed version, not the original.  Things like
transformTemporalClause are going to have user-visible effects: the
renaming you do there will (I think) be reflected in the deparsed
output of views.  That's not good.  Users have a right to expect that
what comes out of deparsing will at least resemble what they put in.

Error reporting might be a problem too: makeTemporalQuerySkeleton is
creating parse nodes that have no location, so if an error develops at
that point, how will the user correlate that with what they typed in?

I suspect there are also problems with plan invalidation.  Any
decisions we make at parse time are fixed forever.  DDL changes can
force a re-plan, but not a re-parse.

Overall, I think that the whole approach here probably needs to be
scrapped and rethought.  The stuff this patch is doing really belongs
in the optimizer, not the parser, I think.  It could possibly happen
at a relatively early stage in the optimizer so that the rest of the
optimizer can see the results of the transformation and, well,
optimize.  But parse time is way too early.

Unrelated to the above, this patch introduces various kinds of helper
functions which are general-purpose in function but dumped in with the
temporal support because it happens to use them.  For instance:

+static Form_pg_type
+typeGet(Oid id)
+{
+HeapTupletp;
+Form_pg_type typtup;
+
+tp = SearchSysCache1(TYPEOID, ObjectIdGetDatum(id));
+if (!HeapTupleIsValid(tp))
+ereport(ERROR,
+(errcode(ERROR),
+ errmsg("cache lookup failed for type %u", id)));
+
+typtup = (Form_pg_type) GETSTRUCT(tp);
+ReleaseSysCache(tp);
+return typtup;
+}

A function as general as typeGet() certainly does not belong in

Re: [HACKERS] GatherMerge misses to push target list

[Robert Haas]

On Mon, Sep 18, 2017 at 7:02 AM, Rushabh Lathia
 wrote:
>> In that case, can you please mark the patch [1] as ready for committer in
>> CF app
>
> Done.

I think this patch is mostly correct, but I think the change to
planner.c isn't quite right.  ordered_rel->reltarget is just a dummy
target list that produces nothing.  Instead, I think we should pass
path->pathtarget, representing the idea that whatever Gather Merge
produces as output is the same as what you put into it.

See attached.

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


pushdown-gathermerge-tlist.patch
Description: Binary data

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

Re: [HACKERS] [Patch] Log SSL certificate verification errors

[Graham Leggett]

On 11 Nov 2017, at 6:23 AM, Michael Paquier  wrote:

>> Currently neither the server side nor the client side SSL certificate verify 
>> callback does anything, leading to potential hair-tearing-out moments.
>> 
>> The following patch to master implements logging of all certificate 
>> verification failures, as well as (crucially) which certificates failed to 
>> verify, and at what depth, so the admin can zoom in straight onto the 
>> problem without any guessing.
> 
> Could you attach as a file to this thread a patch that can be easily
> applied? Using git --format-patch or simply diff is just fine.

I’ve attached it as a separate attachment.

The default behaviour of patch is to ignore all lines before and after the 
patch, so you can use my entire email as an input to patch and it will work 
(This is what git format-patch does, create something that looks like an email).

> Here are also some community guidelines on the matter:
> https://wiki.postgresql.org/wiki/Submitting_a_Patch
> 
> And if you are looking for feedback, you should register it to the
> next commit fest:
> https://commitfest.postgresql.org/16/

I shall do!

Regards,
Graham
—


postgresql-log-cert-verification.diff
Description: Binary data


smime.p7s
Description: S/MIME cryptographic signature

Re: [HACKERS] [PATCH] A hook for session start

[Michael Paquier]

On Sat, Nov 11, 2017 at 12:49 AM, Fabrízio de Royes Mello
 wrote:
> New version attached.

Thanks.

+++ b/src/test/modules/Makefile
  test_extensions \
+ test_session_hooks \
  test_parser
Better if that's in alphabetical order.

That's a nit though, so I am switching the patch as ready for committer.
-- 
Michael


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

Re: [HACKERS] [Patch] Log SSL certificate verification errors

[Michael Paquier]

On Sat, Nov 11, 2017 at 3:34 AM, Graham Leggett  wrote:
> Currently neither the server side nor the client side SSL certificate verify 
> callback does anything, leading to potential hair-tearing-out moments.
>
> The following patch to master implements logging of all certificate 
> verification failures, as well as (crucially) which certificates failed to 
> verify, and at what depth, so the admin can zoom in straight onto the problem 
> without any guessing.

Could you attach as a file to this thread a patch that can be easily
applied? Using git --format-patch or simply diff is just fine.

Here are also some community guidelines on the matter:
https://wiki.postgresql.org/wiki/Submitting_a_Patch

And if you are looking for feedback, you should register it to the
next commit fest:
https://commitfest.postgresql.org/16/
-- 
Michael


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

Re: [HACKERS] pg_upgrade to clusters with a different WAL segment size

[Michael Paquier]

On Sat, Nov 11, 2017 at 12:46 AM, Bossart, Nathan  wrote:
> Allowing changes to the WAL segment size during pg_upgrade seems like a
> nice way to avoid needing a dump and load, so I would like to propose
> adding support for this.  I'd be happy to submit patches for this in the
> next commitfest.

That's a worthy goal.
-- 
Michael


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

Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks

[Michael Paquier]

On Sat, Nov 11, 2017 at 12:50 AM, Robert Haas  wrote:
> On Fri, Nov 10, 2017 at 10:34 AM, Tom Lane  wrote:
>> I think as far as that goes, we can just change to "Therefore, by default
>> their use is restricted ...".  Then I suggest adding a  para
>> after that, with wording along the lines of
>>
>> It is possible to GRANT use of server-side lo_import and lo_export to
>> non-superusers, but careful consideration of the security implications
>> is required.  A malicious user of such privileges could easily parlay
>> them into becoming superuser (for example by rewriting server
>> configuration files), or could attack the rest of the server's file
>> system without bothering to obtain database superuser privileges as
>> such.  Access to roles having such privilege must therefore be guarded
>> just as carefully as access to superuser roles.  Nonetheless, if use
>> of server-side lo_import or lo_export is needed for some routine task,
>> it's safer to use a role of this sort than full superuser privilege,
>> as that helps to reduce the risk of damage from accidental errors.
>
> +1.  That seems like great language to me.

+1. Not convinced that mentioning wrappers is worth the complication.
Experienced admins likely already know such matters.
-- 
Michael


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

Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks

[Stephen Frost]

Michael, Tom,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Fri, Nov 10, 2017 at 10:00 AM, Tom Lane  wrote:
> > Stephen Frost  writes:
> >> I'm guessing no, which essentially means that *we* consider access to
> >> lo_import/lo_export to be equivilant to superuser and therefore we're
> >> not going to implement anything to try and prevent the user who has
> >> access to those functions from becoming superuser.  If we aren't willing
> >> to do that, then how can we really say that there's some difference
> >> between access to these functions and being a superuser?
> >
> > We seem to be talking past each other.  Yes, if a user has malicious
> > intentions, it's possibly to parlay lo_export into obtaining a superuser
> > login (I'm less sure that that's necessarily true for lo_import).
> > That does NOT make it "equivalent", except perhaps in the view of someone
> > who is only considering blocking malevolent actors.  It does not mean that
> > there's no value in preventing a task that needs to run lo_export from
> > being able to accidentally destroy any data in the database.  There's a
> > range of situations where you are concerned about accidents and errors,
> > not malicious intent; but your argument ignores those use-cases.
> 
> That will not sound much as a surprise as I spawned the original
> thread, but like Robert I understand that getting rid of all superuser
> checks is a goal that we are trying to reach to allow admins to have
> more flexibility in handling permissions to a subset of objects.
> Forcing an admin to give full superuser rights to one user willing to
> work only on LOs import and export is a wrong concept.

The problem I have with this is that 'full superuser rights' actually
are being granted by this.  You're able to read any file and write any
file on the filesystem that the PG OS user can.  How is that not 'full
superuser rights'?  The concerns about a mistake being made are just as
serious as they are with someone who has superuser rights as someone
could pretty easily end up overwriting the control file or various other
extremely important bits of the system.  This isn't just about what
'blackhats' can do with this.

As I mentioned up-thread, if we want to make it so that non-superusers
can use lo_import/lo_export, then we should do that in a way that
allows the administrator to specify exactly the rights they really want
to give, based on a use-case for them.  There's no use-case for allowing
someone to use lo_export() to overwrite pg_control.  The use-case would
be to allow them to export to a specific directory (or perhaps a set of
directories), or to read from same.  The same is true of COPY.  Further,
I wonder what would happen if we allow this and then someone comes along
and re-proposes the 'CREATE DIRECTORY' concept that I had once proposed
(ideally with things cleaned up and tightened up to avoid there being
issues using it) to address the actual use-case for these functions to
be available to a non-superuser.  We wouldn't be able to change these
functions to start checking the 'directory' rights or we would break
existing non-superuser usage of them.  I imagine we could create
additional functions which check the 'directory' privileges, but that's
hardly ideal, imv.

I'm disappointed to apparently be in the minority on this, but that
seems to be the case unless someone else wishes to comment.  While I
appreciate the discussion, I'm certainly no more convinced today than I
was when I first saw this go in that allowing these functions to be
granted to non-superusers does anything but effectively make them into
superusers who aren't explicitly identified as superusers.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Proposal: Improve bitmap costing for lossy pages

[Robert Haas]

On Thu, Nov 9, 2017 at 3:55 AM, amul sul  wrote:
> It took me a little while to understand this calculation.  You have moved this
> code from tbm_create(), but I think you should move the following
> comment as well:

I made an adjustment that I hope will address your concern here, made
a few other adjustments, and committed this.

One point of concern that wasn't entirely addressed in the above
discussion is the accuracy of this formula:

+   lossy_pages = Max(0, heap_pages - maxentries / 2);

When I first looked at Dilip's test results, I thought maybe this
formula was way off.  But on closer study, the formula does a decent
(not fantastic) job of estimating the number of exact pages.  The fact
that the number of lossy pages is off is just because the Mackert and
Lohman formula is overestimating how many pages are fetched.  Now, in
Dilip's results, this formula more often than not - but not invariably
- predicted more exact pages than we actually got.  So possibly
instead of maxentries / 2 we could subtract maxentries or some other
multiple of maxentries.  I don't know what's actually best here, but I
think there's a strong argument that this is an improvement as it
stands, and we can adjust it later if it becomes clear what would be
better.

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


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

Re: [HACKERS] LDAP URI decoding bugs

[Thomas Munro]

On Sat, Nov 11, 2017 at 8:37 AM, Peter Eisentraut
 wrote:
> On 11/6/17 23:30, Michael Paquier wrote:
>> On Fri, Nov 3, 2017 at 12:57 PM, Thomas Munro
>>  wrote:
>>> 1.  If you set up a pg_hba.conf with a URL that lacks a base DN or
>>> hostname, hba.c will segfault on startup when it tries to pstrdup a
>>> null pointer.  Examples: ldapurl="ldap://localhost; and
>>> ldapurl="ldap://;.
>>>
>>> 2.  If we fail to bind but have no binddn configured, we'll pass NULL
>>> to ereport (snprint?) for %s, which segfaults on some libc
>>> implementations.  That crash requires more effort to reproduce but you
>>> can see pretty clearly a few lines above in auth.c that it can be
>>> NULL.  (I'm surprised Coverity didn't complain about that.  Maybe it
>>> can't see this code due to macros.)
>
> committed and backpatched

Thanks!

I suppose someone might eventually want to go further and teach it to
understand such bare URLs or missing options (ie leaving out any bits
you want and falling back to the ldap library's defaults, which come
from places like env variables, .ldaprc and /etc/ldap.conf, the way
that "ldapsearch" and other tools manage to work with reasonable
defaults, or at least only need to be set up in one place for all your
LDAP-client software).  I'm not planning to work on that.

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


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

Re: [HACKERS] parallelize queries containing initplans

[Tom Lane]

Robert Haas  writes:
> On Fri, Nov 10, 2017 at 2:24 PM, Tom Lane  wrote:
>> Also, I wonder whether the InvalidOid hack in SS_assign_special_param
>> requires commentary.  It might be safer to use a valid type OID there,
>> perhaps VOIDOID or INTERNALOID.

> There is existing precedent for using InvalidOid to denote the absence
> of a parameter -- cf. copyParamList, SerializeParamList.

OK, fair enough.  I was concerned that this was adding a corner case
not otherwise seen with Params, but evidently not.

regards, tom lane


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

Re: [HACKERS] Planning counters in pg_stat_statements

[Thomas Munro]

On Tue, Nov 7, 2017 at 6:39 PM, Tsunakawa, Takayuki
 wrote:
> From: pgsql-hackers-ow...@postgresql.org
>> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Thomas Munro
>> I have often wanted $SUBJECT and was happy to find that Fujii-san had posted
>> a patch five years ago[1].  The reception then seemed positive.
>> So here is a refurbished and (hopefully) improved version of his patch with
>> a new column for the replan count.  Thoughts?
>
> That's a timely proposal.  I sometimes faced performance problems where the 
> time pg_stat_statements shows is much shorter than the application perceives. 
>  The latest experience was that the execution time of a transaction, which 
> consists of dozens of DMLs and COMMIT, was about 200ms from the application's 
> perspective, while pg_stat_statements showed only about 10ms in total.  The 
> network should not be the cause because the application ran on the same host 
> as the database server.  I wanted to know how long the parsing and planning 
> time was.

Note that this patch doesn't include the parse or parse analysis
times.  I guess they would be less interesting?  But perhaps someone
would want to have the complete query production line measured.

BTW the reason I was looking into this was because an Oracle user
asked me how to see "hard parse" times on Postgres, and I've talked to
others who seem strangely concerned with "parsing" time.  On Oracle I
believe that term covers (among other things) actually planning, and I
guess planning is the most interesting component.  Planning is the
thing I've wanted to measure myself, to diagnose problems relating to
partition/inheritance planning and join explosions and to figure out
which things should be changed to PREPARE/EXECUTE.  Perhaps a separate
parse/analysis counter might become more interesting for us if we ever
add automatic plan cache so you could assess how often you're getting
an implicit prepared statement (something like Oracle's "soft parse")?

> BTW, the current pg_stat_statement shows unexpected time for COMMIT.  I 
> expect it to include the whole COMMIT processing, including the long WAL 
> flush and sync rep wait.  However, it only shows the time for the transaction 
> state change in memory.

That's an interesting point.  You could install a transaction hook to
measure that easily enough, but I'm not sure how useful it'd be: you'd
be grouping together COMMIT timing data from transactions that are
doing very different things (including nothing).  Would that tell you
anything actionable?  If you include commit time for COMMIT statements
then you'd also have to decide whether to include it for DML
statements that run in an implicit transaction.  The trouble with that
is that the same statement inside an explicit transaction wouldn't
have any commit time, so you'd be mixing oranges and apples.  I guess
you could fix that by putting adding "commits" and  "commit_time"
columns (= counters for this statement run as implicit transaction),
but I wonder if commit time monitoring really belongs somewhere else.

For sync rep waits, that's what the pg_stat_replication.XXX_lag
columns tell you.

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


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

Re: [HACKERS] parallelize queries containing initplans

[Robert Haas]

On Fri, Nov 10, 2017 at 2:24 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> I decided to try instead teaching the planner to keep track of the
>> types of PARAM_EXEC parameters as they were created, and that seems to
>> work fine.  See 0001, attached.
>
> I did not look at the other part, but 0001 looks reasonable to me.

Thanks for looking.

> I might quibble with the grammar in the generate_new_param comment:
>
> - * need to record the PARAM_EXEC slot number as being allocated.
> + * need to make sure we have record the type in paramExecTypes (otherwise,
> + * there won't be a slot allocated for it).
>   */
>
> I'd just go with "need to record the type in ..."

Noted.

> Also, I wonder whether the InvalidOid hack in SS_assign_special_param
> requires commentary.  It might be safer to use a valid type OID there,
> perhaps VOIDOID or INTERNALOID.

I think it's pretty straightforward -- if, as the existing comments
say, no Param node will be present and no value will be stored, then
we do not and cannot record the type of the value that we're not
storing.

There is existing precedent for using InvalidOid to denote the absence
of a parameter -- cf. copyParamList, SerializeParamList.  That
convention was not invented by me or the parallel query stuff,
although it has become more widespread for that reason.  I am
disinclined to have this patch invent a New Way To Do It.

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


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

Re: [HACKERS] LDAP URI decoding bugs

[Peter Eisentraut]

On 11/6/17 23:30, Michael Paquier wrote:
> On Fri, Nov 3, 2017 at 12:57 PM, Thomas Munro
>  wrote:
>> 1.  If you set up a pg_hba.conf with a URL that lacks a base DN or
>> hostname, hba.c will segfault on startup when it tries to pstrdup a
>> null pointer.  Examples: ldapurl="ldap://localhost; and
>> ldapurl="ldap://;.
>>
>> 2.  If we fail to bind but have no binddn configured, we'll pass NULL
>> to ereport (snprint?) for %s, which segfaults on some libc
>> implementations.  That crash requires more effort to reproduce but you
>> can see pretty clearly a few lines above in auth.c that it can be
>> NULL.  (I'm surprised Coverity didn't complain about that.  Maybe it
>> can't see this code due to macros.)

committed and backpatched

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


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

Re: [HACKERS] Restrict concurrent update/delete with UPDATE of partition key

[Robert Haas]

On Wed, Sep 27, 2017 at 7:07 AM, amul sul  wrote:
> Attaching POC patch that throws an error in the case of a concurrent update
> to an already deleted tuple due to UPDATE of partition key[1].
>
> In a normal update new tuple is linked to the old one via ctid forming
> a chain of tuple versions but UPDATE of partition key[1] move tuple
> from one partition to an another partition table which breaks that chain.

This patch needs a rebase.  It has one whitespace-only hunk that
should possibly be excluded.

I think the basic idea of this is sound.  Either you or Amit need to
document the behavior in the user-facing documentation, and it needs
tests that hit every single one of the new error checks you've added
(obviously, the tests will only work in combination with Amit's
patch).  The isolation should be sufficient to write such tests.

It needs some more extensive comments as well.  The fact that we're
assigning a meaning to ip_blkid -> InvalidBlockNumber is a big deal,
and should at least be documented in itemptr.h in the comments for the
ItemPointerData structure.

I suspect ExecOnConflictUpdate needs an update for the
HeapTupleUpdated case similar to what you've done elsewhere.

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


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

Re: [HACKERS] parallelize queries containing initplans

[Tom Lane]

Robert Haas  writes:
> I decided to try instead teaching the planner to keep track of the
> types of PARAM_EXEC parameters as they were created, and that seems to
> work fine.  See 0001, attached.

I did not look at the other part, but 0001 looks reasonable to me.
I might quibble with the grammar in the generate_new_param comment:

- * need to record the PARAM_EXEC slot number as being allocated.
+ * need to make sure we have record the type in paramExecTypes (otherwise,
+ * there won't be a slot allocated for it).
  */

I'd just go with "need to record the type in ..."

Also, I wonder whether the InvalidOid hack in SS_assign_special_param
requires commentary.  It might be safer to use a valid type OID there,
perhaps VOIDOID or INTERNALOID.

regards, tom lane


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

Re: [HACKERS] Add some const decorations to prototypes

[Peter Eisentraut]

On 11/10/17 10:29, Fabien COELHO wrote:
> Or basically all is fine, I'm just nitpicking for nothing, shame on me.
> 
> As I said, I rather like more precise declarations.

committed

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


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

Re: [HACKERS] parallelize queries containing initplans

[Robert Haas]

On Tue, Nov 7, 2017 at 4:45 AM, Amit Kapila  wrote:
> As mentioned, changed the status of the patch in CF app.

I spent some time reviewing this patch today and found myself still
quite uncomfortable with the fact that it was adding execution-time
work to track the types of parameters - types that would usually not
even be used.  I found the changes to nodeNestLoop.c to be
particularly objectionable, because we could end up doing the work
over and over when it is actually not needed at all, or at most once.
I decided to try instead teaching the planner to keep track of the
types of PARAM_EXEC parameters as they were created, and that seems to
work fine.  See 0001, attached.

0002, attached, is my worked-over version of the rest of the patch.  I
moved the code that serializes and deserializes PARAM_EXEC from
nodeSubplan.c -- which seemed like a strange choice - to
execParallel.c.  I removed the type OID from the serialization format
because there's no reason to do that any more; the worker already
knows the types from the plan.  I did some renaming of the functions
involved and some adjustment of the comments to refer to "PARAM_EXEC
parameters" instead of initPlan parameters, because there's no reason
that I can see why this can only work for initPlans.  A Gather node on
the inner side of a nested loop doesn't sound like a great idea, but I
think this infrastructure could handle it (though it would need some
more planner work).  I broke a lot of long lines in your version of
the patch into multiple lines; please try to be attentive to this
issue when writing patches in general, as it is a bit tedious to go
through and insert line breaks in many places.

Please let me know your thoughts on the attached patches.

Thanks,

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


0001-param-exec-types-v1.patch
Description: Binary data


0002-pq-pushdown-initplan-rebased.patch
Description: Binary data

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

Re: [HACKERS] Add some const decorations to prototypes

[Peter Eisentraut]

On 11/10/17 11:42, Fabien COELHO wrote:
> After your explanation, and on third thoughts, ISTM that the assignment 
> should not include "const" in the explicit cast, i.e., use
> 
>extern void * msg_func(void);
>const char * msg = (char *) msg_func();
> 
> The variable or field is constant, not what the function returns, so
> 
>const char * msg = (const char *) msg_func();
> 
> does not really make full sense to me, and moreover the compiler does not 
> complain without the const.

The compiler knows how to handle the char * -> const char * case, but
not the char ** -> const char ** case.

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


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

Re: [HACKERS] Fix bloom WAL tap test

[Tom Lane]

I wrote:
> Is there anything we can do to cut the runtime of the TAP test to
> the point where running it by default wouldn't be so painful?

As an experiment, I tried simply cutting the size of the test table 10X:

diff --git a/contrib/bloom/t/001_wal.pl b/contrib/bloom/t/001_wal.pl
index 1b319c9..566abf9 100644
--- a/contrib/bloom/t/001_wal.pl
+++ b/contrib/bloom/t/001_wal.pl
@@ -57,7 +57,7 @@ $node_standby->start;
 $node_master->safe_psql("postgres", "CREATE EXTENSION bloom;");
 $node_master->safe_psql("postgres", "CREATE TABLE tst (i int4, t text);");
 $node_master->safe_psql("postgres",
-"INSERT INTO tst SELECT i%10, substr(md5(i::text), 1, 1) FROM 
generate_series(1,10) i;"
+"INSERT INTO tst SELECT i%10, substr(md5(i::text), 1, 1) FROM 
generate_series(1,1) i;"
 );
 $node_master->safe_psql("postgres",
"CREATE INDEX bloomidx ON tst USING bloom (i, t) WITH (col1 = 3);");
@@ -72,7 +72,7 @@ for my $i (1 .. 10)
test_index_replay("delete $i");
$node_master->safe_psql("postgres", "VACUUM tst;");
test_index_replay("vacuum $i");
-   my ($start, $end) = (11 + ($i - 1) * 1, 10 + $i * 1);
+   my ($start, $end) = (10001 + ($i - 1) * 1000, 1 + $i * 1000);
$node_master->safe_psql("postgres",
 "INSERT INTO tst SELECT i%10, substr(md5(i::text), 1, 1) FROM 
generate_series($start,$end) i;"
);

This about halved the runtime of the TAP test, and it changed the coverage
footprint not at all according to lcov.  (Said coverage is only marginally
better than what we get without running the bloom TAP test, AFAICT.)

It seems like some effort could be put into both shortening this test
and improving the amount of code it exercises.

regards, tom lane


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

Re: [HACKERS] Fix bloom WAL tap test

[Tom Lane]

Michael Paquier  writes:
> On Thu, Nov 9, 2017 at 7:51 PM, Alexander Korotkov
>  wrote:
>> OK, then so be it :)

> Thanks for the new version. This one, as well as the switch to
> psql_safe in 
> https://www.postgresql.org/message-id/CAPpHfduxgEYF_0BTs-mxGC4=w5sw8rnUbq9BSTp1Wq7=nwr...@mail.gmail.com
> are good for a committer lookup IMO.

The safe_psql change is a clear bug fix, so I've pushed it.

However, as far as adding the TAP test to the standard test suite
goes, I've got two complaints about this patch:

1. It doesn't (I think, can't test) do anything to run the test on
Windows.

2. The TAP test is enormously slower than the standard test.  On my
development workstation, "make installcheck" in contrib/bloom takes
about 0.5 sec; this patch increases that to 11.6 sec.  I'm not too
happy about that as a developer, and even less so as an owner of some
fairly slow buildfarm critters.  I'd say that this might be the
reason the TAP test wasn't part of the standard tests to begin with.

Is there anything we can do to cut the runtime of the TAP test to
the point where running it by default wouldn't be so painful?

regards, tom lane


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

Re: [HACKERS] Add some const decorations to prototypes

[Tom Lane]

Fabien COELHO  writes:
>> LWLockTrancheArray = (char **)
>>  MemoryContextAllocZero(TopMemoryContext,
>> LWLockTranchesAllocated * sizeof(char *));

> After your explanation, and on third thoughts, ISTM that the assignment 
> should not include "const" in the explicit cast,

Can't get terribly excited about that one way or the other.  I think
the statement would be OK as-is, and it would also be fine as

LWLockTrancheArray = (const char **)
MemoryContextAllocZero(TopMemoryContext,
   LWLockTranchesAllocated * sizeof(const char *));

The other two possible combinations are not good of course --- not that
they'd generate invalid code, but that they'd require readers to expend
brain cells convincing themselves that the code wasn't wrong.

> ... and moreover the compiler does not 
> complain without the const.

Arguing on the basis of what your compiler does is a pretty shaky basis.
It's not impossible that someone else's compiler would complain if the
casted-to type isn't identical to the variable's type.  I tend to agree
that a compiler *should* allow "char **" to be cast to "const char **"
silently, but that isn't necessarily what happens in the real world.

regards, tom lane


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

Re: [HACKERS] Add some const decorations to prototypes

[Fabien COELHO]

Hello Tom,


ISTM That there is still at least one strange cast:

+static const char **LWLockTrancheArray = NULL;
+   LWLockTrancheArray = (const char **) // twice



These are not cases of "cheating".  This is just the return value of a
memory allocation function being cast from void * to the appropriate
result type.  That is an orthogonal style decision that I have
maintained in these cases.



Would it make sense that the function returns "const void *", i.e. the
cast is not on the const part but on the pointer type part?


Certainly not -- if malloc-like functions returned "const void *" then
you could never write on allocated space without having casted away
const.


Ok. Sure.


   LWLockTrancheArray = (char **)
   MemoryContextAllocZero(TopMemoryContext,
  LWLockTranchesAllocated * sizeof(char *));

and the reader can see by inspection that the calculation of how much
to allocate (so many char-* items) is consistent with the char-**
result.  It is not necessary to go find the declaration of
LWLockTrancheArray and verify that it's char **, because we trust the
compiler to whine if char ** isn't assignment-compatible with that.
But if we left off the cast and just assigned the function result directly
to the variable, then there would be nothing directly tying the variable's
type to this allocation computation.


Thanks for the reflesher course about the intricacies of "const".

After your explanation, and on third thoughts, ISTM that the assignment 
should not include "const" in the explicit cast, i.e., use


  extern void * msg_func(void);
  const char * msg = (char *) msg_func();

The variable or field is constant, not what the function returns, so

  const char * msg = (const char *) msg_func();

does not really make full sense to me, and moreover the compiler does not 
complain without the const.


--
Fabien.


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

Re: [HACKERS] Incorrect comment for build_child_join_rel

[Robert Haas]

On Fri, Nov 10, 2017 at 4:34 AM, Etsuro Fujita
 wrote:
> Here is a small patch for $Subject.

Good catch.  Committed.

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


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

Re: [HACKERS] proposal: psql command \graw

[Pavel Stehule]

2017-11-10 16:38 GMT+01:00 Fabien COELHO :

>
> Hello,
>
> Maybe I'm missing something, but it looks that it could be made to work
>>> without adding another boolean.
>>>
>>
>> The tuples only cannot be disabled, because then other parts print number
>> of rows
>>
>> postgres=# \pset format unaligned
>> Output format is unaligned.
>>
>> postgres=# select 10 as a, 20 as b;
>> a|b
>> 10|20
>> (1 row) <
>>
>
> Argh. Too bad.
>
> I'm not at ease with having two bools which nearly mean the opposite one
> of the other but not exactly... however I'm not sure that there is a
> simpler way out of this, some exception handling is needed one way or the
> other, either within the header or within the footer... Maybe the whole
> topt logic should be reviewed, but that is not the point of this patch.
>

I don't think so it is not correct - this mean tuples only + header.
Probably the best implementation is something three state - all, tuples
only, tuples only and header. But it mean much more changes in psql logic -
not adequate to size of this patch


> So I switched the patch to "ready for committer".
>

Thank you very much

Regards

Pavel


>
> --
> Fabien.
>

Re: [HACKERS] Aggregates push-down to partitions

[Konstantin Knizhnik]

On 10.11.2017 12:15, Ashutosh Bapat wrote:

Maybe in this thread[1] your described problem are solved through

introducing Parallel Append node?

1.
https://www.postgresql.org/message-id/CAJ3gD9dy0K_E8r727heqXoBmWZ83HwLFwdcaSSmBQ1%2BS%2BvRuUQ%40mail.gmail.com

You may want to review [2] and [3] as well.

[2] https://www.postgresql.org/message-id/9666.1491295317@localhost
[3] 
https://www.postgresql.org/message-id/CAM2+6=V64_xhstVHie0Rz=kpeqnljmzt_e314p0jat_oj9m...@mail.gmail.com

Thank you very much for this references.
I applied partition-wise-agg-v6 patches and for partitioned tables it 
works perfectly:


shard=# explain select count(*) from orders;
  QUERY PLAN
---
 Finalize Aggregate  (cost=100415.29..100415.30 rows=1 width=8)
   ->  Append  (cost=50207.63..100415.29 rows=2 width=8)
 ->  Partial Aggregate  (cost=50207.63..50207.64 rows=1 width=8)
   ->  Foreign Scan on orders_0 (cost=101.00..50195.13 
rows=5000 width=0)

 ->  Partial Aggregate  (cost=50207.63..50207.64 rows=1 width=8)
   ->  Foreign Scan on orders_1 (cost=101.00..50195.13 
rows=5000 width=0)

(6 rows)

But I wonder why the same optimization is not applied to normal 
inherited table:


shard=# explain select count(*) from base;
    QUERY PLAN
--
 Aggregate  (cost=44087.99..44088.00 rows=1 width=8)
   ->  Append  (cost=0.00..39079.46 rows=2003414 width=0)
 ->  Seq Scan on base  (cost=0.00..0.00 rows=1 width=0)
 ->  Seq Scan on derived1  (cost=0.00..14425.00 rows=100 
width=0)
 ->  Seq Scan on derived2  (cost=0.00..14425.00 rows=100 
width=0)
 ->  Foreign Scan on derived_fdw  (cost=100.00..212.39 
rows=3413 width=0)

(6 rows)

Are there some principle problems?

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



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

Re: [HACKERS] Add some const decorations to prototypes

[Tom Lane]

Fabien COELHO  writes:
>>> ISTM That there is still at least one strange cast:
 +static const char **LWLockTrancheArray = NULL;
 +   LWLockTrancheArray = (const char **) // twice

>> These are not cases of "cheating".  This is just the return value of a
>> memory allocation function being cast from void * to the appropriate
>> result type.  That is an orthogonal style decision that I have
>> maintained in these cases.

> Would it make sense that the function returns "const void *", i.e. the 
> cast is not on the const part but on the pointer type part?

Certainly not -- if malloc-like functions returned "const void *" then
you could never write on allocated space without having casted away
const.

In any case, what's shown above is not involving any funny stuff,
because the type of LWLockTrancheArray is pointer to non-const
array of pointers to const char strings.  So it's correct to be
assigning a non-const pointer to it.  If it were written like
"const char * const *" then the issues would be quite different.

As for your followup --- yes, we could just omit the cast altogether
and the compiler wouldn't complain, but that is not better style IMO.
The value of the cast really is to document what type we're expecting
the expression to produce.  In context, that statement (today) is

LWLockTrancheArray = (char **)
MemoryContextAllocZero(TopMemoryContext,
   LWLockTranchesAllocated * sizeof(char *));

and the reader can see by inspection that the calculation of how much
to allocate (so many char-* items) is consistent with the char-**
result.  It is not necessary to go find the declaration of
LWLockTrancheArray and verify that it's char **, because we trust the
compiler to whine if char ** isn't assignment-compatible with that.
But if we left off the cast and just assigned the function result directly
to the variable, then there would be nothing directly tying the variable's
type to this allocation computation.

regards, tom lane


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

Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks

[Robert Haas]

On Fri, Nov 10, 2017 at 10:34 AM, Tom Lane  wrote:
> I think as far as that goes, we can just change to "Therefore, by default
> their use is restricted ...".  Then I suggest adding a  para
> after that, with wording along the lines of
>
> It is possible to GRANT use of server-side lo_import and lo_export to
> non-superusers, but careful consideration of the security implications
> is required.  A malicious user of such privileges could easily parlay
> them into becoming superuser (for example by rewriting server
> configuration files), or could attack the rest of the server's file
> system without bothering to obtain database superuser privileges as
> such.  Access to roles having such privilege must therefore be guarded
> just as carefully as access to superuser roles.  Nonetheless, if use
> of server-side lo_import or lo_export is needed for some routine task,
> it's safer to use a role of this sort than full superuser privilege,
> as that helps to reduce the risk of damage from accidental errors.

+1.  That seems like great language to me.

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


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

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Thu, Nov 9, 2017 at 9:08 PM, Michael Paquier 
wrote:
>
> On Fri, Nov 10, 2017 at 2:32 AM, Fabrízio de Royes Mello
>  wrote:
> > On Thu, Nov 9, 2017 at 12:09 AM, Michael Paquier <
michael.paqu...@gmail.com>
> > wrote:
> >> +++ b/src/test/modules/test_session_hooks/session_hooks.conf
> >> @@ -0,0 +1 @@
> >> +shared_preload_libraries = 'test_session_hooks'
> >> Don't you think that this should be a GUC? My previous comment
> >> outlined that. I won't fight hard on that point in any case, don't
> >> worry. I just want to make things clear :)
> >
> > Ooops... my fault... fixed!
> >
> > Thanks again!!
>
> +/* GUC variables */
> +static char *session_hook_username = NULL;
> This causes the module to crash if test_session_hooks.username is not
> set. I would recommend just assigning a default value, say "postgres".
>

Fixed.

New version attached.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 05c5c19..d3156ad 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -169,6 +169,9 @@ static ProcSignalReason RecoveryConflictReason;
 static MemoryContext row_description_context = NULL;
 static StringInfoData row_description_buf;
 
+/* Hook for plugins to get control at start of session */
+session_start_hook_type session_start_hook = NULL;
+
 /* 
  *		decls for routines only used in this file
  * 
@@ -3857,6 +3860,9 @@ PostgresMain(int argc, char *argv[],
 	if (!IsUnderPostmaster)
 		PgStartTime = GetCurrentTimestamp();
 
+	if (session_start_hook)
+		(*session_start_hook) ();
+
 	/*
 	 * POSTGRES main processing loop begins here
 	 *
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 20f1d27..16ec376 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -76,6 +76,8 @@ static bool ThereIsAtLeastOneRole(void);
 static void process_startup_options(Port *port, bool am_superuser);
 static void process_settings(Oid databaseid, Oid roleid);
 
+/* Hook for plugins to get control at end of session */
+session_end_hook_type session_end_hook = NULL;
 
 /*** InitPostgres support ***/
 
@@ -1154,6 +1156,10 @@ ShutdownPostgres(int code, Datum arg)
 	 * them explicitly.
 	 */
 	LockReleaseAll(USER_LOCKMETHOD, true);
+
+	/* Hook at session end */
+	if (session_end_hook)
+		(*session_end_hook) ();
 }
 
 
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index f8c535c..9f05bfb 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -35,6 +35,13 @@ extern PGDLLIMPORT const char *debug_query_string;
 extern int	max_stack_depth;
 extern int	PostAuthDelay;
 
+/* Hook for plugins to get control at start and end of session */
+typedef void (*session_start_hook_type) (void);
+typedef void (*session_end_hook_type) (void);
+
+extern PGDLLIMPORT session_start_hook_type session_start_hook;
+extern PGDLLIMPORT session_end_hook_type session_end_hook;
+
 /* GUC-configurable parameters */
 
 typedef enum
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index b7ed0af..a3c8c1e 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -11,6 +11,7 @@ SUBDIRS = \
 		  snapshot_too_old \
 		  test_ddl_deparse \
 		  test_extensions \
+		  test_session_hooks \
 		  test_parser \
 		  test_pg_dump \
 		  test_rbtree \
diff --git a/src/test/modules/test_session_hooks/.gitignore b/src/test/modules/test_session_hooks/.gitignore
new file mode 100644
index 000..5dcb3ff
--- /dev/null
+++ b/src/test/modules/test_session_hooks/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/test_session_hooks/Makefile b/src/test/modules/test_session_hooks/Makefile
new file mode 100644
index 000..c5c3860
--- /dev/null
+++ b/src/test/modules/test_session_hooks/Makefile
@@ -0,0 +1,21 @@
+# src/test/modules/test_session_hooks/Makefile
+
+MODULES = test_session_hooks
+PGFILEDESC = "test_session_hooks - Test session hooks with an extension"
+
+EXTENSION = test_session_hooks
+DATA = test_session_hooks--1.0.sql
+
+REGRESS = test_session_hooks
+REGRESS_OPTS = --temp-config=$(top_srcdir)/src/test/modules/test_session_hooks/session_hooks.conf
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/test_session_hooks
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git 

Re: [HACKERS] proposal: psql command \graw

[Fabien COELHO]

Hello,

Maybe I'm missing something, but it looks that it could be made to work 
without adding another boolean.


The tuples only cannot be disabled, because then other parts print number
of rows

postgres=# \pset format unaligned
Output format is unaligned.

postgres=# select 10 as a, 20 as b;
a|b
10|20
(1 row) <


Argh. Too bad.

I'm not at ease with having two bools which nearly mean the opposite one 
of the other but not exactly... however I'm not sure that there is a 
simpler way out of this, some exception handling is needed one way or the 
other, either within the header or within the footer... Maybe the whole 
topt logic should be reviewed, but that is not the point of this patch.


So I switched the patch to "ready for committer".

--
Fabien.


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

Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks

[Tom Lane]

Michael Paquier  writes:
> That will not sound much as a surprise as I spawned the original
> thread, but like Robert I understand that getting rid of all superuser
> checks is a goal that we are trying to reach to allow admins to have
> more flexibility in handling permissions to a subset of objects.
> Forcing an admin to give full superuser rights to one user willing to
> work only on LOs import and export is a wrong concept.

Right.  I think the question may boil down to how we document this.
The current para reads

The server-side lo_import and
lo_export functions behave considerably differently
from their client-side analogs.  These two functions read and write files
in the server's file system, using the permissions of the database's
owning user.  Therefore, their use is restricted to superusers.  In
contrast, the client-side import and export functions read and write files
in the client's file system, using the permissions of the client program.
The client-side functions do not require superuser privilege.

I think as far as that goes, we can just change to "Therefore, by default
their use is restricted ...".  Then I suggest adding a  para
after that, with wording along the lines of

It is possible to GRANT use of server-side lo_import and lo_export to
non-superusers, but careful consideration of the security implications
is required.  A malicious user of such privileges could easily parlay
them into becoming superuser (for example by rewriting server
configuration files), or could attack the rest of the server's file
system without bothering to obtain database superuser privileges as
such.  Access to roles having such privilege must therefore be guarded
just as carefully as access to superuser roles.  Nonetheless, if use
of server-side lo_import or lo_export is needed for some routine task,
it's safer to use a role of this sort than full superuser privilege,
as that helps to reduce the risk of damage from accidental errors.

We could expand that by mentioning the possibility of wrapper functions,
but it seems long enough already.

Comments?

regards, tom lane


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

Re: [HACKERS] Add some const decorations to prototypes

[Fabien COELHO]

Would it make sense that the function returns "const void *", i.e. the cast 
is not on the const part but on the pointer type part?


Or maybe you do not really need a cast, the following code does not 
generate any warning when compiled with clang & gcc.


 #include 

 // const void * would be ok as well
 void * msg_fun(void)
 {
   return "hello world";
 }

 int main(void)
 {
   const char * msg = msg_fun();
   printf("message: %s\n", msg);
   return 0;
 }

Or basically all is fine, I'm just nitpicking for nothing, shame on me.

As I said, I rather like more precise declarations.

--
Fabien.


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

Re: [HACKERS] Add some const decorations to prototypes

[Fabien COELHO]

ISTM That there is still at least one strange cast:

  +static const char **LWLockTrancheArray = NULL;
  +   LWLockTrancheArray = (const char **) // twice


These are not cases of "cheating".  This is just the return value of a
memory allocation function being cast from void * to the appropriate
result type.  That is an orthogonal style decision that I have
maintained in these cases.


Ok. I'm at the limit of my C abilities.

Your answer is about void * vs char *, I'm okay with that.

My question was about no const / const in the same operation.

Would it make sense that the function returns "const void *", i.e. the 
cast is not on the const part but on the pointer type part?


--
Fabien.


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

Re: [HACKERS] PATCH: multivariate histograms and MCV lists

[Mark Dilger]

> On Sep 12, 2017, at 2:06 PM, Tomas Vondra  
> wrote:
> 
> Attached is an updated version of the patch, dealing with fallout of
> 821fb8cdbf700a8aadbe12d5b46ca4e61be5a8a8 which touched the SGML
> documentation for CREATE STATISTICS.

Your patches need updating.

Tom's commit 471d55859c11b40059aef7dd82f82b3a0dc338b1 changed 
src/bin/psql/describe.c, which breaks your 0001-multivariate-MCV-lists.patch.gz
file.

I reviewed the patch a few months ago, and as I recall, it looked good to me.
I should review it again before approving it, though.

mark



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

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

[Robert Haas]

On Fri, Nov 10, 2017 at 5:44 AM, Amit Kapila  wrote:
> I am seeing the assertion failure as below on executing the above
> mentioned Create statement:
>
> TRAP: FailedAssertion("!(!(tup->t_data->t_infomask & 0x0008))", File:
> "heapam.c", Line: 2634)
> server closed the connection unexpectedly
> This probably means the server terminated abnormally

OK, I see it now.  Not sure why I couldn't reproduce this before.

I think the problem is not actually with the code that I just wrote.
What I'm seeing is that the slot descriptor's tdhasoid value is false
for both the funnel slot and the result slot; therefore, we conclude
that no projection is needed to remove the OIDs.  That seems to make
sense: if the funnel slot doesn't have OIDs and the result slot
doesn't have OIDs either, then we don't need to remove them.
Unfortunately, even though the funnel slot descriptor is marked
tdhashoid = false, the tuples being stored there actually do have
OIDs.  And that is because they are coming from the underlying
sequential scan, which *also* has OIDs despite the fact that tdhasoid
for it's slot is false.

This had me really confused until I realized that there are two
processes involved.  The problem is that we don't pass eflags down to
the child process -- so in the user backend, everybody agrees that
there shouldn't be OIDs anywhere, because EXEC_FLAG_WITHOUT_OIDS is
set.  In the parallel worker, however, it's not set, so the worker
feels free to do whatever comes naturally, and in this test case that
happens to be returning tuples with OIDs.  Patch for this attached.

I also noticed that the code that initializes the funnel slot is using
its own PlanState rather than the outer plan's PlanState to call
ExecContextForcesOids.  I think that's formally incorrect, because the
goal is to end up with a slot that is the same as the outer plan's
slot.  It doesn't matter because ExecContextForcesOids doesn't care
which PlanState it gets passed, but the comments in
ExecContextForcesOids imply that somebody it might, so perhaps it's
best to clean that up.  Patch for this attached, too.

And here are the other patches again, too.

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


0001-pass-eflags-to-worker-v1.patch
Description: Binary data


0002-forces-oids-neatnikism-v1.patch
Description: Binary data


0003-skip-gather-project-v2.patch
Description: Binary data


0004-shm-mq-less-spinlocks-v2.patch
Description: Binary data


0005-shm-mq-reduce-receiver-latch-set-v1.patch
Description: Binary data


0006-remove-memory-leak-protection-v1.patch
Description: Binary data

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

Re: [HACKERS] Variable substitution in psql backtick expansion

[Pavel Stehule]

Hi

I am sending a review of last patch psql-server-version-1.patch.gz

This patch is trivial - the most big problem is choosing correct name for
GUC. I am thinking so server_version_raw is acceptable.

I had to fix doc - see attached updated patch

All tests passed.

I'll mark this patch as ready for commiters

Regards

Pavel
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index d360fc4d58..924766fce7 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -7956,8 +7956,22 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
   
   

-Reports the version number of the server as an integer. It is determined
-by the value of PG_VERSION_NUM when building the server.
+Reports the version number of the server as a short string. It is determined
+by the value of PG_VERSION when building the server.
+   
+  
+ 
+
+ 
+  server_version_raw (string)
+  
+   server_version_raw configuration parameter
+  
+  
+  
+   
+Reports the version of the server as a long string. It is determined
+by the value of PG_VERSION_STR when building the server.

   
  
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index e520cdf3ba..50d6f0a8fc 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3770,11 +3770,14 @@ bar
   
 
   
+SERVER_VERSION
 SERVER_VERSION_NAME
 SERVER_VERSION_NUM
 
 
-The server's version number as a string, for
+The server's version number as a long string, for
+example PostgreSQL 11devel ...,
+as a short string, for
 example 9.6.2, 10.1 or 11beta1,
 and in numeric form, for
 example 90602 or 11.
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index c4c1afa084..49ff61246f 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -500,6 +500,7 @@ static char *locale_collate;
 static char *locale_ctype;
 static char *server_encoding_string;
 static char *server_version_string;
+static char *server_version_raw_string;
 static int	server_version_num;
 static char *timezone_string;
 static char *log_timezone_string;
@@ -3295,6 +3296,18 @@ static struct config_string ConfigureNamesString[] =
 		NULL, NULL, NULL
 	},
 
+	{
+		/* Can't be set in postgresql.conf */
+		{"server_version_raw", PGC_INTERNAL, PRESET_OPTIONS,
+			gettext_noop("Shows the server version string."),
+			NULL,
+			GUC_REPORT | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+		},
+		_version_raw_string,
+		PG_VERSION_STR,
+		NULL, NULL, NULL
+	},
+
 	{
 		/* Not for general use --- used by SET ROLE */
 		{"role", PGC_USERSET, UNGROUPED,
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 8cc4de3878..cfac89c8da 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -3210,7 +3210,8 @@ void
 SyncVariables(void)
 {
 	char		vbuf[32];
-	const char *server_version;
+	const char *server_version,
+			   *server_version_raw;
 
 	/* get stuff from connection */
 	pset.encoding = PQclientEncoding(pset.db);
@@ -3237,6 +3238,17 @@ SyncVariables(void)
 	snprintf(vbuf, sizeof(vbuf), "%d", pset.sversion);
 	SetVariable(pset.vars, "SERVER_VERSION_NUM", vbuf);
 
+	server_version_raw = PQparameterStatus(pset.db, "server_version_raw");
+	/* fall back again */
+	if (!server_version_raw)
+	{
+		snprintf(vbuf, sizeof(vbuf), "PostgreSQL ");
+		formatPGVersionNumber(pset.sversion, true, vbuf + strlen(vbuf),
+			  sizeof(vbuf) - strlen(vbuf));
+		server_version_raw = vbuf;
+	}
+	SetVariable(pset.vars, "SERVER_VERSION", server_version_raw);
+
 	/* send stuff to it, too */
 	PQsetErrorVerbosity(pset.db, pset.verbosity);
 	PQsetErrorContextVisibility(pset.db, pset.show_context);
@@ -3255,6 +3267,7 @@ UnsyncVariables(void)
 	SetVariable(pset.vars, "HOST", NULL);
 	SetVariable(pset.vars, "PORT", NULL);
 	SetVariable(pset.vars, "ENCODING", NULL);
+	SetVariable(pset.vars, "SERVER_VERSION", NULL);
 	SetVariable(pset.vars, "SERVER_VERSION_NAME", NULL);
 	SetVariable(pset.vars, "SERVER_VERSION_NUM", NULL);
 }
diff --git a/src/interfaces/libpq/fe-protocol2.c b/src/interfaces/libpq/fe-protocol2.c
index 5335a91440..0418779f79 100644
--- a/src/interfaces/libpq/fe-protocol2.c
+++ b/src/interfaces/libpq/fe-protocol2.c
@@ -280,6 +280,10 @@ pqSetenvPoll(PGconn *conn)
 		{
 			char	   *ptr;
 
+			/* keep returned value */
+			pqSaveParameterStatus(conn, "server_version_raw",
+  val);
+
 			/* strip off PostgreSQL part */
 			val += 11;
 
diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out
index 43ac5f5f11..eabb990d4e 100644
--- a/src/test/regress/expected/guc.out
+++ b/src/test/regress/expected/guc.out
@@ -767,3 +767,14 @@ NOTICE:  text search configuration "no_such_config" does not exist
 select 

Re: [HACKERS] Add some const decorations to prototypes

[Peter Eisentraut]

On 11/4/17 16:50, Fabien COELHO wrote:
>>> Just leave it as char*.  If you change the endptr argument you're going to
>>> force every call site to change their return variable, and some of them
>>> would end up having to cast away the const on their end.
>>
>> OK, here is an updated patch with the controversial bits removed.
> 
> I'm in general favor in helping compilers, but if you have to cheat.
> 
> ISTM That there is still at least one strange cast:
> 
>   +static const char **LWLockTrancheArray = NULL;
>   +   LWLockTrancheArray = (const char **) // twice

These are not cases of "cheating".  This is just the return value of a
memory allocation function being cast from void * to the appropriate
result type.  That is an orthogonal style decision that I have
maintained in these cases.

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


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

Re: [HACKERS] Transform for pl/perl

[Pavel Stehule]

Hi

2017-10-24 14:27 GMT+02:00 Anthony Bykov :

> There are some moments I should mention:
> 1. {"1":1}::jsonb is transformed into HV {"1"=>"1"}, while
> ["1","2"]::jsonb is transformed into AV ["1", "2"]
>
> 2. If there is a numeric value appear in jsonb, it will be transformed
> to SVnv through string (Numeric->String->SV->SVnv). Not the best
> solution, but as far as I understand this is usual practise in
> postgresql to serialize Numerics and de-serialize them.
>
> 3. SVnv is transformed into jsonb through string
> (SVnv->String->Numeric).
>
> An example may also be helpful to understand extension. So, as an
> example, function "test" transforms incoming jsonb into perl,
> transforms it back into jsonb and returns it.
>
> create extension jsonb_plperl cascade;
>
> create or replace function test(val jsonb)
> returns jsonb
> transform for type jsonb
> language plperl
> as $$
> return $_[0];
> $$;
>
> select test('{"1":1,"example": null}'::jsonb);
>
>
I am looking to this patch:

1. the patch contains some artefacts - look the word "hstore"

2. I got lot of warnings


make[1]: Vstupuje se do adresáře
„/home/pavel/src/postgresql/contrib/jsonb_plperl“
gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute
-Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard
-g -ggdb -Og -g3 -fno-omit-frame-pointer -fPIC -I../../src/pl/plperl -I.
-I. -I../../src/include  -D_GNU_SOURCE -I/usr/include/libxml2
-I/usr/lib64/perl5/CORE  -c -o jsonb_plperl.o jsonb_plperl.c
jsonb_plperl.c: In function ‘SV_FromJsonbValue’:
jsonb_plperl.c:83:9: warning: ‘result’ may be used uninitialized in this
function [-Wmaybe-uninitialized]
  return (result);
 ^
jsonb_plperl.c: In function ‘SV_FromJsonb’:
jsonb_plperl.c:95:10: warning: ‘object’ may be used uninitialized in this
function [-Wmaybe-uninitialized]
  HV *object;
  ^~
In file included from /usr/lib64/perl5/CORE/perl.h:5644:0,
 from ../../src/pl/plperl/plperl.h:52,
 from jsonb_plperl.c:17:
/usr/lib64/perl5/CORE/embed.h:404:19: warning: ‘value’ may be used
uninitialized in this function [-Wmaybe-uninitialized]
 #define newRV(a)  Perl_newRV(aTHX_ a)
   ^~
jsonb_plperl.c:101:10: note: ‘value’ was declared here
  SV *value;
  ^
gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute
-Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard
-g -ggdb -Og -g3 -fno-omit-frame-pointer -fPIC -shared -o jsonb_plperl.so
jsonb_plperl.o  -L../../src/port -L../../src/common -Wl,--as-needed
-Wl,-rpath,'/usr/lib64/perl5/CORE',--enable-new-dtags   -Wl,-z,relro
-specs=/usr/lib/rpm/redhat/redhat-hardened-ld -fstack-protector-strong
-L/usr/local/lib  -L/usr/lib64/perl5/CORE -lperl -lpthread -lresolv -lnsl
-ldl -lm -lcrypt -lutil -lc
make[1]: Opouští se adresář
„/home/pavel/src/postgresql/contrib/jsonb_plperl“

[pavel@nemesis contrib]$ gcc --version
gcc (GCC) 7.2.1 20170915 (Red Hat 7.2.1-2)
Copyright (C) 2017 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

3. regress tests passed

4. There are not any documentation - probably it should be part of PLPerl

5. The regress tests doesn't coverage other datatypes than numbers. I miss
boolean, binary, object, ... Maybe using data::dumper or some similar can
be interesting

Note - it is great extension, I am pleasured so transformations are used.

Regards

Pavel







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

Re: [HACKERS] pg_basebackup --progress output for batch execution

[Martín Marqués]

Hi,

Thanks for having a look at this patch.

2017-11-09 20:55 GMT-03:00 Jeff Janes :
> On Fri, Sep 29, 2017 at 4:00 PM, Martin Marques
>  wrote:
>>
>> Hi,
>>
>> Some time ago I had to work on a system where I was cloning a standby
>> using pg_basebackup, that didn't have screen or tmux. For that reason I
>> redirected the output to a file and ran it with nohup.
>>
>> I normally (always actually ;) ) run pg_basebackup with --progress and
>> --verbose so I can follow how much has been done. When done on a tty you
>> get a nice progress bar with the percentage that has been cloned.
>>
>> The problem came with the execution and redirection of the output, as
>> the --progress option will write a *very* long line!
>>
>> Back then I thought of writing a patch (actually someone suggested I do
>> so) to add a --batch-mode option which would change the behavior
>> pg_basebackup has when printing the output messages.
>
>
>
> While separate lines in the output file is better than one very long line,
> it still doesn't seem so useful.  If you aren't watching it in real time,
> then you really need to have a timestamp on each line so that you can
> interpret the output.  The lines are about one second apart, but I don't
> know robust that timing is under all conditions.

I kind of disagree with your view here.

If the cloning process takes many hours to complete (in my case, it
was around 12 hours IIRC) you might want to peak at the log every now
and then with tail.

I do agree on adding a timestamp prefix to each line, as it's not
clear from the code how often progress_report() is called.

> I think I agree with Arthur that I'd rather have the decision made by
> inspecting whether output is going to a tty, rather than by adding another
> command line option.  But maybe that is not detected robustly enough across
> all platforms and circumstances?

In this case, Arthur's idea is good, but would make the patch less
generic/flexible for the end user.

That's why I tried to reproduce what top does when executed with -b
(Batch mode operation). There, it's the end user who decides how the
output is formatted (well, saying it decides on formatting a bit of an
overstatement, but you get it ;) )

An example where using isatty() might fail is if you run pg_basebackup
from a tty but redirect the output to a file, I believe that in that
case isatty() will return true, but it's very likely that the user
might want batch mode output.

But maybe we should also add Arthurs idea anyway (when not in batch
mode), as it seems pretty lame to output progress in one line if you
are not in a tty.

Thoughts?

-- 
Martín Marquéshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] Another oddity in handling of WCO constraints in postgres_fdw

[Etsuro Fujita]

(2017/11/01 11:16), Robert Haas wrote:

On Wed, Oct 4, 2017 at 5:58 PM, Ashutosh Bapat
  wrote:

The view with WCO is local but the modification which violates WCO is
being made on remote server by a trigger on remote table. Trying to
control that doesn't seem to be a good idea, just like we can't
control what rows get inserted on the foreign server when they violate
local constraints.


I think that's a fair point.


For local constraints on foreign tables, it's the user's responsibility 
to ensure that those constraints matches the remote side, so we don't 
need to ensure those constraints locally.  But I'm not sure if the same 
thing applies to WCOs on views defined on foreign tables, because in 
some case it's not possible to impose constraints on the remote side 
that match those WCOs, as I explained before.


Best regards,
Etsuro Fujita


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

Re: [HACKERS] Runtime Partition Pruning

[amul sul]

On Thu, Nov 9, 2017 at 4:48 PM, Beena Emerson  wrote:
> Hello all,
>
> Here is the updated patch which is rebased over v10 of Amit Langote's
> path towards faster pruning patch [1]. It modifies the PartScanKeyInfo
> struct to hold expressions which is then evaluated by the executor to
> fetch the correct partitions using the function.
>

Hi Beena,

I have started looking into your patch, here few initial comments
for your 0001 patch:

1.
 351 + * Evaluate and store the ooutput of ExecInitExpr for each
of the keys.

Typo: ooutput

2.
 822 +   if (IsA(constexpr, Const) &_runtime)
 823 +   continue;
 824 +
 825 +   if (IsA(constexpr, Param) &&!is_runtime)
 826 +   continue;
 827 +

 Add space after '&&'

 3.
 1095 +* Generally for appendrel we don't fetch the clause from the the

Typo: Double 'the'

4.
 272 
-/*-
 273 + 
/*-

 Unnecessary hunk.

5.
 313 +   Node   *n =
eval_const_expressions_from_list(estate->es_param_list_info, val);
 314 +

Crossing 80 column window.  Same at line # 323 & 325

6.
 315 +   keys->eqkeys_datums[i++] = ((Const *) n)->constvalue;

Don’t we need a check for IsA(n, Const) or assert ?

7.
1011 +   if (prmList)
1012 +   context.boundParams = prmList;  /* bound Params */
1013 +   else
1014 +   context.boundParams = NULL;

No need of prmList null check, context.boundParams = prmList; is enough.

8.  It would be nice if you create a separate patch where you are moving
PartScanKeyInfo and exporting function declaration.

9.  Could you please add few regression tests, that would help in
review & testing.

10. Could you please rebase your patch against latest "path toward faster
partition pruning" patch by Amit.

Regards,
Amul


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

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

[Amit Kapila]

On Fri, Nov 10, 2017 at 9:48 AM, Amit Kapila  wrote:
> On Fri, Nov 10, 2017 at 8:36 AM, Robert Haas  wrote:
>> On Thu, Nov 9, 2017 at 9:31 PM, Amit Kapila  wrote:
>>> Have you set force_parallel_mode=regress; before running the
>>> statement?
>>
>> Yes, I tried that first.
>>
>>> If so, then why you need to tune other parallel query
>>> related parameters?
>>
>> Because I couldn't get it to break the other way, I then tried this.
>>
>> Instead of asking me what I did, can you tell me what I need to do?
>> Maybe a self-contained reproducible test case including exactly what
>> goes wrong on your end?
>>
>
> I think we are missing something very basic because you should see the
> failure by executing that statement in force_parallel_mode=regress
> even in a freshly created database.
>

I am seeing the assertion failure as below on executing the above
mentioned Create statement:

TRAP: FailedAssertion("!(!(tup->t_data->t_infomask & 0x0008))", File:
"heapam.c", Line: 2634)
server closed the connection unexpectedly
This probably means the server terminated abnormally


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


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

Re: [HACKERS] pg_basebackup --progress output for batch execution

[Arthur Zakirov]

On Thu, Nov 09, 2017 at 03:55:36PM -0800, Jeff Janes wrote:
> 
> I think I agree with Arthur that I'd rather have the decision made by
> inspecting whether output is going to a tty, rather than by adding another
> command line option.  But maybe that is not detected robustly enough across
> all platforms and circumstances?
> 

isatty() is used within Postgres code already (for example, pg_upgrade/util.c).
However, it seems that on Windows isatty() is deprecated and it is recommended 
to use _isatty(). Moreover, on Windows it can give false positive result [1], 
if I'm not mistaken:

> The _isatty function determines whether fd is associated with a character 
> device (a terminal, console, printer, or serial port).

1 - https://msdn.microsoft.com/en-us/library/f4s0ddew(v=vs.140).aspx

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


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

Re: [HACKERS] Aggregates push-down to partitions

[Ashutosh Bapat]

On Fri, Nov 10, 2017 at 12:20 AM, Maksim Milyutin  wrote:
> Hi Konstantin!

>> I wonder if somebody already investigate this problem or working in this
>> direction.
>> May be there are already some patches proposed?
>> I have searched hackers archive, but didn't find something relevant...
>> Are there any suggestions about the best approach to implement this
>> feature?
>>
>
> Maybe in this thread[1] your described problem are solved through
> introducing Parallel Append node?
>
> 1.
> https://www.postgresql.org/message-id/CAJ3gD9dy0K_E8r727heqXoBmWZ83HwLFwdcaSSmBQ1%2BS%2BvRuUQ%40mail.gmail.com

You may want to review [2] and [3] as well.

[2] https://www.postgresql.org/message-id/9666.1491295317@localhost
[3] 
https://www.postgresql.org/message-id/CAM2+6=V64_xhstVHie0Rz=kpeqnljmzt_e314p0jat_oj9m...@mail.gmail.com
-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


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

Re: [HACKERS] UPDATE of partition key

[Amit Khandekar]

On 9 November 2017 at 09:27, Thomas Munro  wrote:
> On Wed, Nov 8, 2017 at 5:57 PM, Amit Khandekar  wrote:
>> On 8 November 2017 at 07:55, Thomas Munro  
>> wrote:
>>> On Tue, Nov 7, 2017 at 8:03 AM, Robert Haas  wrote:
 The changes to trigger.c still make me super-nervous.  Hey THOMAS
 MUNRO, any chance you could review that part?
>
> At first, it seemed quite strange to me that row triggers and
> statement triggers fire different events for the same modification.
> Row triggers see DELETE +  INSERT (necessarily because different
> tables are involved), but this fact is hidden from the target table's
> statement triggers.
>
> The alternative would be for all triggers to see consistent events and
> transitions.  Instead of having your special case code in ExecInsert
> and ExecDelete that creates the two halves of a 'synthetic' UPDATE for
> the transition tables, you'd just let the existing ExecInsert and
> ExecDelete code do its thing, and you'd need a flag to record that you
> should also fire INSERT/DELETE after statement triggers if any rows
> moved.

Yeah I also had thought about that. But thought that change was too
invasive. For e.g. letting ExecARInsertTriggers() do the transition
capture even when transition_capture->tcs_update_new_table is set.

I was also thinking of having a separate function to *only* add the
transition table rows. So in ExecInsert, call this one instead of
ExecARUpdateTriggers(). But realized that the existing
ExecARUpdateTriggers() looks like a better, robust interface with all
its checks. Just that calling ExecARUpdateTriggers() sounds like we
are also firing trigger; we are not firing any trigger or saving any
event, we are just adding the transition row.

>
> After sleeping on this question, I am coming around to the view that
> the way you have it is right.  The distinction isn't really between
> row triggers and statement triggers, it's between triggers at
> different levels in the hierarchy.  It just so happens that we
> currently only fire target table statement triggers and leaf table row
> triggers.

Yes. And rows are there only in leaf partitions. So we have to
simulate as though the target table has these rows. Like you
mentioned, the user has to get the impression of a normal table. So we
have to do something extra to capture the rows.

> Future development ideas that seem consistent with your choice:
>
> 1.  If we ever allow row triggers with transition tables on child
> tables, then I think *their* transition tables should certainly see
> the deletes and inserts, otherwise OLD TABLE and NEW TABLE would be
> inconsistent with the OLD and NEW variables in a single trigger
> invocation.  (These were prohibited mainly due to lack of time and
> (AFAIK) limited usefulness; I think they would need probably need
> their own separate tuplestores, or possibly some kind of filtering.)

As we know, for row triggers on leaf partitions, we treat them as
normal tables, so a trigger written on a leaf partition sees only the
local changes. The trigger is unaware whether the insert is part of an
UPDATE row movement. Similarly, the transition table referenced by
that row trigger function should see only the NEW table, not the old
table.

>
> 2.  If we ever allow row triggers on partitioned tables (ie that fire
> when its children are modified), then I think their UPDATE trigger
> should probably fire when a row moves between any two (grand-)*child
> tables, just as you have it for target table statement triggers.

Yes I agree.

> It doesn't matter that the view from parent tables' triggers is
> inconsistent with the view from leaf table triggers: it's a feature
> that we 'hide' partitioning from the user to the extent we can so that
> you can treat the partitioned table just like a table.
>
> Any other views?

I think because because there is no provision for a row trigger on
partitioned table, users who want to have a common trigger on a
partition subtree, has no choice but to create the same trigger
individually on the leaf partitions. And that's the reason we cannot
handle an update row movement with triggers without anomalies.

Thanks
-Amit Khandekar


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

Re: [HACKERS] path toward faster partition pruning

[David Rowley]

On 10 November 2017 at 16:30, Kyotaro HORIGUCHI
 wrote:
> In 0002, bms_add_range has a bit naive-looking loop
>
> +   while (wordnum <= uwordnum)
> +   {
> +   bitmapword mask = (bitmapword) ~0;
> +
> +   /* If working on the lower word, zero out bits below 'lower'. 
> */
> +   if (wordnum == lwordnum)
> +   {
> +   int lbitnum = BITNUM(lower);
> +   mask >>= lbitnum;
> +   mask <<= lbitnum;
> +   }
> +
> +   /* Likewise, if working on the upper word, zero bits above 
> 'upper' */
> +   if (wordnum == uwordnum)
> +   {
> +   int ushiftbits = BITS_PER_BITMAPWORD - (BITNUM(upper) 
> + 1);
> +   mask <<= ushiftbits;
> +   mask >>= ushiftbits;
> +   }
> +
> +   a->words[wordnum++] |= mask;
> +   }
>
> Without some aggressive optimization, the loop takes most of the
> time to check-and-jump for nothing especially with many
> partitions and somewhat unintuitive.
>
> The following uses a bit tricky bitmap operation but
> is straightforward as a whole.
>
> =
> /* fill the bits upper from BITNUM(lower) (0-based) of the first word */
> a->workds[wordnum++] += ~(bitmapword)((1 << BITNUM(lower)) - 1);
>
> /* fill up intermediate words */
> while (wordnum < uwordnum)
>a->words[wordnum++] = ~(bitmapword) 0;
>
> /* fill up to BITNUM(upper) bit (0-based) of the last word */
> a->workds[wordnum++] |=
>  (~(bitmapword) 0) >> (BITS_PER_BITMAPWORD - (BITNUM(upper) - 1));
> =

No objections here for making bms_add_range() perform better, but this
is not going to work when lwordnum == uwordnum. You'd need to special
case that. I didn't think it was worth the trouble, but maybe it is...

I assume the += should be |=.

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


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

Re: [HACKERS] proposal: psql command \graw

[Pavel Stehule]

2017-11-10 8:12 GMT+01:00 Fabien COELHO :

>
> ISTM that you can remove "force_column_header" and just set "tuple_only"
>>> to what you need, that is you do not need to change anything in function
>>> "print_unaligned_text".
>>>
>>
>> Last point is not possible - I would not to break original tuple only
>> mode.
>>
>
> Hmmm... I do not understand. I can see only one use of force_column_header
> in the function:
>
>  -   if (!opt_tuples_only)
>  +   if (!opt_tuples_only || opt_force_column_header)
>
> So I would basically suggest to do:
>
>  my_popt.topt.tuples_only = !pset.g_raw_header;
>
> in the driver. Looking at the detailed code in that function, probably you
> need to set start_table to on when headers are needed and stop_table to off
> for the raw mode anyway?
>
> Maybe I'm missing something, but it looks that it could be made to work
> without adding another boolean.
>

The tuples only cannot be disabled, because then other parts print number
of rows

postgres=# \pset format unaligned
Output format is unaligned.

postgres=# select 10 as a, 20 as b;
a|b
10|20
(1 row) <



> --
> Fabien.
>

Re: [HACKERS] proposal: psql command \graw

[Fabien COELHO]

ISTM that you can remove "force_column_header" and just set "tuple_only"
to what you need, that is you do not need to change anything in function
"print_unaligned_text".


Last point is not possible - I would not to break original tuple only mode.


Hmmm... I do not understand. I can see only one use of force_column_header 
in the function:


 -   if (!opt_tuples_only)
 +   if (!opt_tuples_only || opt_force_column_header)

So I would basically suggest to do:

 my_popt.topt.tuples_only = !pset.g_raw_header;

in the driver. Looking at the detailed code in that function, probably you 
need to set start_table to on when headers are needed and stop_table to 
off for the raw mode anyway?


Maybe I'm missing something, but it looks that it could be made to work 
without adding another boolean.


--
Fabien.


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

Re: [HACKERS] path toward faster partition pruning

[Kyotaro HORIGUCHI]

Hello,

At Fri, 10 Nov 2017 14:44:55 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20171110.144455.117208639.horiguchi.kyot...@lab.ntt.co.jp>

> > Those two conditions are not orthogonal. Maybe something like
> > following seems more understantable.
> > 
> > > if (!constfalse)
> > > {
> > >   /* No constraints on the keys, so, return *all* partitions. */
> > >   if (nkeys == 0)
> > > return bms_add_range(result, 0, partdesc->nparts - 1);
> > > 
> > >   result = get_partitions_for_keys(relation, );
> > > }

So, the condition (!constfalse && nkeys == 0) cannot return
there. I'm badly confused by the variable name.

I couldn't find another reasonable structure using the current
classify_p_b_keys(), but could you add a comment like the
following as an example?

+ /*
+  * Ths function processes other than OR expressions and returns
+  * the excluded OR expressions in or_clauses
+  */
>   nkeys = classify_partition_bounding_keys(relation, clauses,
>, ,
>_clauses);
>   /*
>* Only look up in the partition decriptor if the query provides
>* constraints on the keys at all.
>*/
>   if (!constfalse)
>   {
> if (nkey > 0)
>   result = get_partitions_for_keys(relation, );
> else
-+   /* No constraints on the keys, so, all partitions are passed. */
>   result = bms_add_range(result, 0, partdesc->nparts - 1);
>   }
> 
+   /*
+* We have a partition set for clauses not returned in or_clauses
+* here. Conjuct the result of each OR clauses.
+*/
>   foreach(lc, or_clauses)
>   {
> BoolExpr *or = (BoolExpr *) lfirst(lc);
> ListCell *lc1;
> Bitmapset *or_partset = NULL;
> 
+ Assert(or_clause(or));

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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

Re: [HACKERS] proposal: psql command \graw

[Pavel Stehule]

2017-11-09 21:12 GMT+01:00 Pavel Stehule :

>
>
> 2017-11-09 21:03 GMT+01:00 Fabien COELHO :
>
>>
>> Hello Pavel,
>>
>> I hope so I fixed all mentioned issues.
>>>
>>
>> Patch applies with a warning:
>>
>>  > git apply ~/psql-graw-2.patch
>>  /home/fabien/psql-graw-2.patch:192: new blank line at EOF.
>>  +
>>  warning: 1 line adds whitespace errors.
>>
>> Otherwise it compiles. "make check" ok. doc gen ok.
>>
>> Two spurious empty lines are added before StoreQueryTuple.
>>
>> Doc: "If + is appended to the command name, a column
>> names are displayed."
>>
>> I suggest instead: "When + is appended, column names
>> are also displayed."
>>
>> ISTM that you can remove "force_column_header" and just set "tuple_only"
>> to what you need, that is you do not need to change anything in function
>> "print_unaligned_text".
>>
>
> Last point is not possible - I would not to break original tuple only
> mode.
>
>
updated patch




> Pavel
>
>>
>> --
>> Fabien.
>>
>
>
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index e520cdf3ba..457a59eeab 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2020,6 +2020,19 @@ CREATE INDEX
   
 
 
+  
+\graw[+] [ filename ]
+\graw[+]  [ |command ]
+
+
+\graw is equivalent to \g, but
+forces unaligned output mode for this query. When +
+is appended, column names are also displayed.
+
+
+  
+
+
   
 \gset [ prefix ]
 
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 8cc4de3878..b3461291eb 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -332,7 +332,8 @@ exec_command(const char *cmd,
 		status = exec_command_errverbose(scan_state, active_branch);
 	else if (strcmp(cmd, "f") == 0)
 		status = exec_command_f(scan_state, active_branch);
-	else if (strcmp(cmd, "g") == 0 || strcmp(cmd, "gx") == 0)
+	else if (strcmp(cmd, "g") == 0 || strcmp(cmd, "gx") == 0 ||
+			 strcmp(cmd, "graw") == 0 || strcmp(cmd, "graw+") == 0)
 		status = exec_command_g(scan_state, active_branch, cmd);
 	else if (strcmp(cmd, "gdesc") == 0)
 		status = exec_command_gdesc(scan_state, active_branch);
@@ -1232,6 +1233,7 @@ exec_command_f(PsqlScanState scan_state, bool active_branch)
 
 /*
  * \g [filename] -- send query, optionally with output to file/pipe
+ * \graw [filename] -- same as \g with raw format
  * \gx [filename] -- same as \g, with expanded mode forced
  */
 static backslashResult
@@ -1254,6 +1256,10 @@ exec_command_g(PsqlScanState scan_state, bool active_branch, const char *cmd)
 		free(fname);
 		if (strcmp(cmd, "gx") == 0)
 			pset.g_expanded = true;
+		else if (strcmp(cmd, "graw") == 0)
+			pset.g_raw = true;
+		else if (strcmp(cmd, "graw+") == 0)
+			pset.g_raw_header = true;
 		status = PSQL_CMD_SEND;
 	}
 	else
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 7a91a44b2b..9f7ef51dfb 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -865,6 +865,14 @@ PrintQueryTuples(const PGresult *results)
 	if (pset.g_expanded)
 		my_popt.topt.expanded = 1;
 
+	/* one-shot raw output requested by \raw and \graw+ */
+	else if (pset.g_raw || pset.g_raw_header)
+	{
+		my_popt.topt.format = PRINT_UNALIGNED;
+		my_popt.topt.tuples_only = true;
+		my_popt.topt.force_column_header = pset.g_raw_header;
+	}
+
 	/* write output to \g argument, if any */
 	if (pset.gfname)
 	{
@@ -1517,6 +1525,10 @@ sendquery_cleanup:
 	/* reset \gx's expanded-mode flag */
 	pset.g_expanded = false;
 
+	/* reset \graw flags */
+	pset.g_raw = false;
+	pset.g_raw_header = false;
+
 	/* reset \gset trigger */
 	if (pset.gset_prefix)
 	{
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index a926c40b9b..e573711434 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -167,7 +167,7 @@ slashUsage(unsigned short int pager)
 	 * Use "psql --help=commands | wc" to count correctly.  It's okay to count
 	 * the USE_READLINE line even in builds without that.
 	 */
-	output = PageOutput(125, pager ? &(pset.popt.topt) : NULL);
+	output = PageOutput(126, pager ? &(pset.popt.topt) : NULL);
 
 	fprintf(output, _("General\n"));
 	fprintf(output, _("  \\copyright show PostgreSQL usage and distribution terms\n"));
@@ -176,6 +176,7 @@ slashUsage(unsigned short int pager)
 	fprintf(output, _("  \\g [FILE] or ; execute query (and send results to file or |pipe)\n"));
 	fprintf(output, _("  \\gdesc describe result of query, without executing it\n"));
 	fprintf(output, _("  \\gexec execute query, then execute each value in its result\n"));
+	fprintf(output, _("  \\graw[+] [FILE]as \\g, but forces unaligned raw output mode\n"));
 	fprintf(output, _("  \\gset [PREFIX] execute query and store results in psql variables\n"));
 	fprintf(output, _("  \\gx [FILE] as \\g, but forces expanded output mode\n"));
 	

Re: [HACKERS] path toward faster partition pruning

[Kyotaro HORIGUCHI]

Ooops! The following comment is wrong. Please ignore it.

At Fri, 10 Nov 2017 14:38:11 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20171110.143811.97616847.horiguchi.kyot...@lab.ntt.co.jp>
> Those two conditions are not orthogonal. Maybe something like
> following seems more understantable.
> 
> > if (!constfalse)
> > {
> >   /* No constraints on the keys, so, return *all* partitions. */
> >   if (nkeys == 0)
> > return bms_add_range(result, 0, partdesc->nparts - 1);
> > 
> >   result = get_partitions_for_keys(relation, );
> > }
> 
> I'm not sure what is meant to be (formally) done here, but it
> seems to me that OrExpr is assumed to be only at the top level of
> the caluses. So the following (just an example, but meaningful
> expression in this shpape must exists.) expression is perhaps
> wrongly processed here.
> 
> CREATE TABLE p (a int) PARITION BY (a);
> CREATE TABLE c1 PARTITION OF p FOR VALUES FROM (0) TO (10);
> CREATE TABLE c2 PARTITION OF p FOR VALUES FROM (10) TO (20);
> 
> SELECT * FROM p WHERE a = 15 AND (a = 15 OR a = 5);
> 
> get_partitions_for_keys() returns both c1 and c2 and still
> or_clauses here holds (a = 15 OR a = 5) and the function tries to
> *add* partitions for a = 15 and a = 5 separetely.

This is working fine. Sorry for the bogus comment.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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

Re: [HACKERS] [POC] hash partitioning

[amul sul]

On Fri, Nov 10, 2017 at 4:41 AM, Robert Haas  wrote:
> On Wed, Nov 1, 2017 at 6:16 AM, amul sul  wrote:
>> Fixed in the 0003 patch.
>
> I have committed this patch set with the attached adjustments.
>

Thanks a lot for your support & a ton of thanks to all reviewer.

Regards,
Amul


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

Re: [HACKERS] path toward faster partition pruning

[Kyotaro HORIGUCHI]

Hello, this is the second part of the review.

At Fri, 10 Nov 2017 12:30:00 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20171110.123000.151902771.horiguchi.kyot...@lab.ntt.co.jp>
> In 0002, bms_add_range has a bit naive-looking loop
> In 0003, 

In 0004,

The name get_partitions_from_clauses_guts(), it seems to me that
we usually use _internal for recursive part of some function. (I
have the same comment as David about the comment for
get_partition_from_clause())

About the same function:

Couldn't we get out in the fast path when clauses == NIL?

+/* No constraints on the keys, so, return *all* partitions. */
+result = bms_add_range(result, 0, partdesc->nparts - 1);

This allows us to return immediately here. And just above this,

+   if (nkeys > 0 && !constfalse)
+   result = get_partitions_for_keys(relation, );
+   else if (!constfalse)

Those two conditions are not orthogonal. Maybe something like
following seems more understantable.

> if (!constfalse)
> {
>   /* No constraints on the keys, so, return *all* partitions. */
>   if (nkeys == 0)
> return bms_add_range(result, 0, partdesc->nparts - 1);
> 
>   result = get_partitions_for_keys(relation, );
> }

I'm not sure what is meant to be (formally) done here, but it
seems to me that OrExpr is assumed to be only at the top level of
the caluses. So the following (just an example, but meaningful
expression in this shpape must exists.) expression is perhaps
wrongly processed here.

CREATE TABLE p (a int) PARITION BY (a);
CREATE TABLE c1 PARTITION OF p FOR VALUES FROM (0) TO (10);
CREATE TABLE c2 PARTITION OF p FOR VALUES FROM (10) TO (20);

SELECT * FROM p WHERE a = 15 AND (a = 15 OR a = 5);

get_partitions_for_keys() returns both c1 and c2 and still
or_clauses here holds (a = 15 OR a = 5) and the function tries to
*add* partitions for a = 15 and a = 5 separetely.


I'd like to pause here.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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

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

[Amit Kapila]

On Fri, Nov 10, 2017 at 8:36 AM, Robert Haas  wrote:
> On Thu, Nov 9, 2017 at 9:31 PM, Amit Kapila  wrote:
>> Have you set force_parallel_mode=regress; before running the
>> statement?
>
> Yes, I tried that first.
>
>> If so, then why you need to tune other parallel query
>> related parameters?
>
> Because I couldn't get it to break the other way, I then tried this.
>
> Instead of asking me what I did, can you tell me what I need to do?
> Maybe a self-contained reproducible test case including exactly what
> goes wrong on your end?
>

I think we are missing something very basic because you should see the
failure by executing that statement in force_parallel_mode=regress
even in a freshly created database.  I guess the missing point is that
I am using assertions enabled build and probably you are not (If this
is the reason, then it should have striked me first time).  Anyway, I
am writing steps to reproduce the issue.

1. initdb
2. start server
3. connect using psql
4. set force_parallel_mode=regress;
5. Create Table as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';


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


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

Re: [HACKERS] path toward faster partition pruning

[Kyotaro HORIGUCHI]

Hello,

At Fri, 10 Nov 2017 09:34:57 +0900, Amit Langote 
 wrote in 
<5fcb1a9f-b4ad-119d-14c7-282c30c7f...@lab.ntt.co.jp>
> Hi Amul.
> 
> On 2017/11/09 20:05, amul sul wrote:
> > On Mon, Nov 6, 2017 at 3:31 PM, Amit Langote
> >  wrote:
> >> On 2017/11/06 14:32, David Rowley wrote:
> >>> On 6 November 2017 at 17:30, Amit Langote wrote:
>  On 2017/11/03 13:32, David Rowley wrote:
> > On 31 October 2017 at 21:43, Amit Langote wrote:
> > []
> >>
> >> Attached updated set of patches, including the fix to make the new pruning
> >> code handle Boolean partitioning.
> >>
> > 
> > I am getting following warning on mac os x:
> 
> Thanks for the review.
> 
> > partition.c:1800:24: warning: comparison of constant -1 with
> > expression of type 'NullTestType'
> >   (aka 'enum NullTestType') is always false
> > [-Wtautological-constant-out-of-range-compare]
> > if (keynullness[i] == -1)
> > ~~ ^  ~~
> > partition.c:1932:25: warning: comparison of constant -1 with
> > expression of type 'NullTestType'
> >   (aka 'enum NullTestType') is always false
> > [-Wtautological-constant-out-of-range-compare]
> > if (keynullness[i] == -1)
> > ~~ ^  ~~
> > 2 warnings generated.
> > 
> > 
> > Comment for 0004 patch:
> > 270 +   /* -1 represents an invalid value of NullTestType. */
> >  271 +   memset(keynullness, -1, PARTITION_MAX_KEYS * sizeof(NullTestType));
> > 
> > I think we should not use memset to set a value other than 0 or true/false.
> > This will work for -1 on the system where values are stored in the 2's
> > complement but I am afraid of other architecture.
> 
> OK, I will remove all instances of comparing and setting variables of type
> NullTestType to a value of -1.

In 0002, bms_add_range has a bit naive-looking loop

+   while (wordnum <= uwordnum)
+   {
+   bitmapword mask = (bitmapword) ~0;
+
+   /* If working on the lower word, zero out bits below 'lower'. */
+   if (wordnum == lwordnum)
+   {
+   int lbitnum = BITNUM(lower);
+   mask >>= lbitnum;
+   mask <<= lbitnum;
+   }
+
+   /* Likewise, if working on the upper word, zero bits above 
'upper' */
+   if (wordnum == uwordnum)
+   {
+   int ushiftbits = BITS_PER_BITMAPWORD - (BITNUM(upper) + 
1);
+   mask <<= ushiftbits;
+   mask >>= ushiftbits;
+   }
+
+   a->words[wordnum++] |= mask;
+   }

Without some aggressive optimization, the loop takes most of the
time to check-and-jump for nothing especially with many
partitions and somewhat unintuitive.

The following uses a bit tricky bitmap operation but
is straightforward as a whole.

=
/* fill the bits upper from BITNUM(lower) (0-based) of the first word */
a->workds[wordnum++] += ~(bitmapword)((1 << BITNUM(lower)) - 1);

/* fill up intermediate words */
while (wordnum < uwordnum)
   a->words[wordnum++] = ~(bitmapword) 0;

/* fill up to BITNUM(upper) bit (0-based) of the last word */
a->workds[wordnum++] |=
 (~(bitmapword) 0) >> (BITS_PER_BITMAPWORD - (BITNUM(upper) - 1));
=


In 0003, 

+match_clauses_to_partkey(RelOptInfo *rel,
...
+  if (rinfo->pseudoconstant &&
+(IsA(clause, Const) &&
+ Const *) clause)->constisnull) ||
+  !DatumGetBool(((Const *) clause)->constvalue
+  {
+*constfalse = true;
+continue;

If we call this function in both conjunction and disjunction
context (the latter is only in recursive case). constfalse ==
true means no need of any clauses for the former case.

Since (I think) just a list of RestrictInfo is expected to be
treated as a conjunction (it's quite doubious, though..), we
might be better call this for each subnodes of a disjunction. Or,
like match_clauses_to_index, we might be better provide
match_clause_to_partkey(rel, rinfo, contains_const), which
returns NULL if constfalse. (I'm not self-confident on this..)


+  /*
+   * If no commutator exists, cannot flip the qual's args,
+   * so give up.
+   */
+  if (!OidIsValid(expr_op))
+continue;

I suppose it's better to leftop and rightop as is rather than
flipping over so that var is placed left-side. Does that make
things so complex?

+ * It the operator happens to be '<>', which is never listed
If?


+if (!op_in_opfamily(expr_op, partopfamily))
+{
+  Oidnegator = get_negator(expr_op);
+
+  if (!OidIsValid(negator) ||
+!op_in_opfamily(negator, partopfamily))
+continue;

classify_partition_bounding_keys() checks the same thing by
checking whether the 

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

[Robert Haas]

On Thu, Nov 9, 2017 at 9:31 PM, Amit Kapila  wrote:
> Have you set force_parallel_mode=regress; before running the
> statement?

Yes, I tried that first.

> If so, then why you need to tune other parallel query
> related parameters?

Because I couldn't get it to break the other way, I then tried this.

Instead of asking me what I did, can you tell me what I need to do?
Maybe a self-contained reproducible test case including exactly what
goes wrong on your end?

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


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