[HACKERS] Migration to pglister - Before

[Stephen Frost]

Greetings,

We will be migrating these lists to pglister in the next few minutes.

This final email on the old list system is intended to let you know
that future emails will have different headers and you will need to
adjust your filters.

The changes which we expect to be most significant to users can be found
on the wiki here: https://wiki.postgresql.org/wiki/PGLister_Announce

Once the migration of these lists is complete, an 'after' email will be
sent out.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] UPDATE of partition key

[David Rowley]

ement level DELETE or INSERT as
* these occur on the individual partitions, none of which are the
* target of this the UPDATE statement.
*/

A similar comment could use a similar improvement in ExecDelete()

23. Line is longer than 80 chars.

TransitionCaptureState *transition_capture = mtstate->mt_transition_capture;

24. I know from reading the thread this name has changed before, but I
think delete_skipped seems like the wrong name for this variable in:

if (delete_skipped)
*delete_skipped = true;

Skipped is the wrong word here as that indicates like we had some sort
of choice and that we decided not to. However, that's not the case
when the tuple was concurrently deleted. Would it not be better to
call it "tuple_deleted" or even "success" and reverse the logic? It's
just a bit confusing that you're setting this to skipped before
anything happens. It would be nicer if there was a better way to do
this whole thing as it's a bit of a wart in the code. I understand why
the code exists though.

Also, I wonder if it's better to always pass a boolean here to save
having to test for NULL before setting it, that way you might consider
putting the success = false just before the return NULL, then do
success = true after the tuple is gone.
Failing that, putting: something like: success = false; /* not yet! */
where you're doing the if (deleted_skipped) test is might also be
better.

25. Comment "we should" should be "we must".

/*
* For some reason if DELETE didn't happen (for e.g. trigger
* prevented it, or it was already deleted by self, or it was
* concurrently deleted by another transaction), then we should
* skip INSERT as well, otherwise, there will be effectively one
* new row inserted.

Maybe just:
/* If the DELETE operation was unsuccessful, then we must not
* perform the INSERT into the new partition.

"for e.g." is not really correct in English. "For example, ..." or
just "e.g. ..." is correct. If you de-abbreviate the e.g. then you've
written "For exempli gratia", which translates to "For for example".

26. You're not really explaining what's going on here:

if (mtstate->mt_transition_capture)
saved_tcs_map = mtstate->mt_transition_capture->tcs_map;

You have a comment later to say you're about to "Revert back to the
transition capture map", but I missed the part that explained about
modifying it in the first place.

27. Comment does not explain how we're skipping checking the partition
constraint check in:

* We have already checked partition constraints above, so skip
* checking them here.

Maybe something like:

* We've already checked the partition constraint above, however, we
* must still ensure the tuple passes all other constraints, so we'll
* call ExecConstraints() and have it validate all remaining checks.

28. For table WITH OIDs, the OID should probably follow the new tuple
for partition-key-UPDATEs.

CREATE TABLE p (a BOOL NOT NULL, b INT NOT NULL) PARTITION BY LIST (a)
WITH OIDS;
CREATE TABLE P_true PARTITION OF p FOR VALUES IN('t');
CREATE TABLE P_false PARTITION OF p FOR VALUES IN('f');
INSERT INTO p VALUES('t', 10);
SELECT tableoid::regclass,oid,a FROM p;
 tableoid |  oid  | a
--+---+---
 p_true   | 16792 | t
(1 row)

UPDATE p SET a = 'f'; -- partition-key-UPDATE (oid has changed (it
probably shouldn't have))
SELECT tableoid::regclass,oid,a FROM p;
 tableoid |  oid  | a
--+---+---
 p_false  | 16793 | f
(1 row)

UPDATE p SET b = 20; -- non-partition-key-UPDATE (oid remains the same)

SELECT tableoid::regclass,oid,a FROM p;
 tableoid |  oid  | a
--+---+---
 p_false  | 16793 | f
(1 row)

I'll try to continue with the review tomorrow, but I think some other
reviews are also looming too.

-- 
 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] 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 run out.  If we don't m

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]

ng('"a"'::jsonb);
+
+-- test jsonb null -> python None
+CREATE FUNCTION test1null(val jsonb) RETURNS int
+LANGUAGE plpython2u
+TRANSFORM FOR TYPE jsonb
+AS $$
+assert(val == None)
+return val==None
+$$;
+
+SELECT test1null('"a"'::jsonb);
+
+-- test python -> jsonb
+CREATE FUNCTION back(val jsonb) RETURNS jsonb
+LANGUAGE plpython2u
+TRANSFORM FOR TYPE jsonb
+as $$
+return val
+$$;
+
+SELECT back('null'::jsonb);
+SELECT back('1'::jsonb);
+SELECT back('true'::jsonb);
+SELECT back('"string"'::jsonb);
+
+SELECT back('{"1":null}'::jsonb);
+SELECT back('{"1":1}'::jsonb);
+SELECT back('{"1":true}'::jsonb);
+SELECT back('{"1":"string"}'::jsonb);
+
+SELECT back('[null]'::jsonb);
+SELECT back('[1]'::jsonb);
+SELECT back('[true]'::jsonb);
+SELECT back('["string"]'::jsonb);
+SELECT back('[null,1]'::jsonb);
+SELECT back('[1,true]'::jsonb);
+SELECT back('[true,"string"]'::jsonb);
+SELECT back('["string","string2"]'::jsonb);
+
+
+DROP EXTENSION plpython2u CASCADE;
+
+
+-- Testing plpythonu extension.
+CREATE EXTENSION jsonb_plpythonu CASCADE;
+
+-- test jsonb -> python dict
+CREATE FUNCTION test1(val jsonb) RETURNS int
+LANGUAGE plpythonu
+TRANSFORM FOR TYPE jsonb
+AS $$
+assert isinstance(val, dict)
+plpy.info(sorted(val.items()))
+return len(val)
+$$;
+
+SELECT test1('{"a":1, "c":"NULL"}'::jsonb);
+
+-- test jsonb -> python dict
+-- complex dict with dicts as value
+CREATE FUNCTION test1complex(val jsonb) RETURNS int
+LANGUAGE plpythonu
+TRANSFORM FOR TYPE jsonb
+AS $$
+assert isinstance(val, dict)
+assert(val == {"d":{"d": 1}})
+return len(val)
+$$;
+
+SELECT test1complex('{"d":{"d": 1}}'::jsonb);
+
+
+-- test jsonb[] -> python dict
+-- dict with array as value
+CREATE FUNCTION test1arr(val jsonb) RETURNS int
+LANGUAGE plpythonu
+TRANSFORM FOR TYPE jsonb
+AS $$
+assert isinstance(val, dict)
+assert(val == {"d": [12,1]})
+return len(val)
+$$;
+
+SELECT test1arr('{"d":[12,1]}'::jsonb);
+
+-- test jsonb[] -> python list
+-- simple list
+CREATE FUNCTION test2arr(val jsonb) RETURNS int
+LANGUAGE plpythonu
+TRANSFORM FOR TYPE jsonb
+AS $$
+assert isinstance(val, list)
+assert(val == [12,1])
+return len(val)
+$$;
+
+SELECT test2arr('[12,1]'::jsonb);
+
+-- test jsonb[] -> python list
+-- array of dicts
+CREATE FUNCTION test3arr(val jsonb) RETURNS int
+LANGUAGE plpythonu
+TRANSFORM FOR TYPE jsonb
+AS $$
+assert isinstance(val, list)
+assert(val == [{"a":1,"b":2},{"c":3,"d":4}])
+return len(val)
+$$;
+
+SELECT test3arr('[{"a":1,"b":2},{"c":3,"d":4}]'::jsonb);
+
+-- test jsonb int -> python int
+CREATE FUNCTION test1int(val jsonb) RETURNS int
+LANGUAGE plpythonu
+TRANSFORM FOR TYPE jsonb
+AS $$
+assert(val == [1])
+return len(val)
+$$;
+
+SELECT test1int('1'::jsonb);
+
+-- test jsonb string -> python string
+CREATE FUNCTION test1string(val jsonb) RETURNS int
+LANGUAGE plpythonu
+TRANSFORM FOR TYPE jsonb
+AS $$
+assert(val == ["a"])
+return len(val)
+$$;
+
+SELECT test1string('"a"'::jsonb);
+
+-- test jsonb null -> python None
+CREATE FUNCTION test1null(val jsonb) RETURNS int
+LANGUAGE plpythonu
+TRANSFORM FOR TYPE jsonb
+AS $$
+assert(val == None)
+return val==None
+$$;
+
+SELECT test1null('"a"'::jsonb);
+
+-- test python -> jsonb
+CREATE FUNCTION back(val jsonb) RETURNS jsonb
+LANGUAGE plpythonu
+TRANSFORM FOR TYPE jsonb
+as $$
+return val
+$$;
+
+SELECT back('null'::jsonb);
+SELECT back('1'::jsonb);
+SELECT back('true'::jsonb);
+SELECT back('"string"'::jsonb);
+
+SELECT back('{"1":null}'::jsonb);
+SELECT back('{"1":1}'::jsonb);
+SELECT back('{"1":true}'::jsonb);
+SELECT back('{"1":"string"}'::jsonb);
+
+SELECT back('[null]'::jsonb);
+SELECT back('[1]'::jsonb);
+SELECT back('[true]'::jsonb);
+SELECT back('["string"]'::jsonb);
+SELECT back('[null,1]'::jsonb);
+SELECT back('[1,true]'::jsonb);
+SELECT back('[true,"string"]'::jsonb);
+SELECT back('["string","string2"]'::jsonb);
+
+
+DROP EXTENSION plpythonu CASCADE;
diff --git a/contrib/jsonb_plpython/sql/jsonb_plpython3.sql b/contrib/jsonb_plpython/sql/jsonb_plpython3.sql
new file mode 100644
index 000..eaddf9a
--- /dev/null
+++ b/contrib/jsonb_plpython/sql/jsonb_plpython3.sql
@@ -0,0 +1,129 @@
+CREATE EXTENSION jsonb_plpython3u CASCADE;
+
+-- test jsonb -> python dict
+CREATE FUNCTION test1(val jsonb) RETURNS int
+LANGUAGE plpython3u
+TRANSFORM FOR TYPE jsonb
+AS $$
+assert isinstance(val, dict)
+plpy.info(sorted(val.items()))
+return len(val)
+$$;
+
+SELECT test1('{"a":1, "c":"NULL"}'::jsonb);
+
+-- test jsonb -> python dict
+-- complex dict with dicts as value
+CREATE FUNCTION test1complex(val jsonb) RETURNS int
+LANGUAGE plpython3u
+TRANSFORM FOR TYPE jsonb
+AS $$
+assert isinstance(val, dict)
+assert(val == {"d":{"d": 1}})
+return len(val)
+$$;
+
+SELECT test1complex('{"d":{"d": 1}}'::jsonb);
+
+
+-- test jsonb[] -> python dict
+-- dict with array as value
+CREATE FUNCTION test1arr(val jsonb) RETURNS int
+LANGUAGE plpython3u
+TRANSFORM FOR TYPE jsonb
+AS $$
+assert isinstance(val, dict)
+assert(val == {"d": [12,1]})
+return len(val)
+$$;
+
+SELECT test1arr('{"d":[12,1]}'::jsonb);
+
+-- test jsonb[] -> python list
+-- simple list
+CREATE FUNCTION test2arr(val jsonb) RETURNS int
+LANGUAGE plpython3u
+TRANSFORM FOR TYPE jsonb
+AS $$
+assert isinstance(val, list)
+assert(val == [12,1])
+return len(val)
+$$;
+
+SELECT test2arr('[12,1]'::jsonb);
+
+-- test jsonb[] -> python list
+-- array of dicts
+CREATE FUNCTION test3arr(val jsonb) RETURNS int
+LANGUAGE plpython3u
+TRANSFORM FOR TYPE jsonb
+AS $$
+assert isinstance(val, list)
+assert(val == [{"a":1,"b":2},{"c":3,"d":4}])
+return len(val)
+$$;
+
+SELECT test3arr('[{"a":1,"b":2},{"c":3,"d":4}]'::jsonb);
+
+-- test jsonb int -> python int
+CREATE FUNCTION test1int(val jsonb) RETURNS int
+LANGUAGE plpython3u
+TRANSFORM FOR TYPE jsonb
+AS $$
+assert(val == [1])
+return len(val)
+$$;
+
+SELECT test1int('1'::jsonb);
+
+-- test jsonb string -> python string
+CREATE FUNCTION test1string(val jsonb) RETURNS int
+LANGUAGE plpython3u
+TRANSFORM FOR TYPE jsonb
+AS $$
+assert(val == ["a"])
+return len(val)
+$$;
+
+SELECT test1string('"a"'::jsonb);
+
+-- test jsonb null -> python None
+CREATE FUNCTION test1null(val jsonb) RETURNS int
+LANGUAGE plpython3u
+TRANSFORM FOR TYPE jsonb
+AS $$
+assert(val == None)
+return val==None
+$$;
+
+SELECT test1null('"a"'::jsonb);
+
+-- test python -> jsonb
+CREATE FUNCTION back(val jsonb) RETURNS jsonb
+LANGUAGE plpython3u
+TRANSFORM FOR TYPE jsonb
+as $$
+return val
+$$;
+
+SELECT back('null'::jsonb);
+SELECT back('1'::jsonb);
+SELECT back('true'::jsonb);
+SELECT back('"string"'::jsonb);
+
+SELECT back('{"1":null}'::jsonb);
+SELECT back('{"1":1}'::jsonb);
+SELECT back('{"1":true}'::jsonb);
+SELECT back('{"1":"string"}'::jsonb);
+
+SELECT back('[null]'::jsonb);
+SELECT back('[1]'::jsonb);
+SELECT back('[true]'::jsonb);
+SELECT back('["string"]'::jsonb);
+SELECT back('[null,1]'::jsonb);
+SELECT back('[1,true]'::jsonb);
+SELECT back('[true,"string"]'::jsonb);
+SELECT back('["string","string2"]'::jsonb);
+
+
+DROP EXTENSION plpython3u CASCADE;
diff --git a/doc/src/sgml/json.sgml b/doc/src/sgml/json.sgml
index 05ecef2..4e8a7a0 100644
--- a/doc/src/sgml/json.sgml
+++ b/doc/src/sgml/json.sgml
@@ -569,4 +569,20 @@ SELECT jdoc->'guid', jdoc->'name' FROM api WHERE jdoc @> '{"tags": ["qu
   compared using the default database collation.
   
  
+  
+  Transforms
+
+  
+   Additional extensions are available that implement transforms for
+   the jsonb type for the language PL/Python.  The
+   extensions for PL/Python are called
+   jsonb_plpythonu, jsonb_plpython2u,
+   and jsonb_plpython3u
+   (see  for the PL/Python naming
+   convention).  If you use them, jsonb values are mapped to
+   Python dictionaries.
+  
+ 
+
+
 

-- 
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

[Amit Langote]

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() accepts such a clause with a <> operator,
then classify_partition_bounding_keys() can rely that it will get the
desired btree operators to implement pruning for the same.

> # In the function, "partkey strategy" and "operator strategy" are
> # confusing..

I agree it would be better to make that clear using comments.

Partitioning strategy and operator strategies are intimately related.
List and range partitioning related optimizations will only work if the
clause operators are of valid btree strategies, hash partitioning
optimizations will only work if the operator in the matched clauses is a
valid hash equality operator.


> +  AttrNumber  attno;
> 
> This declaration might be better in a narrower scope.

Agreed, will move.

On 2017/11/10 14:38, Kyotaro HORIGUCHI wrote:
> Hello, this is the second part of the review.
>
> At Fri, 10 Nov 2017 12:30:00 +0900 , Kyotaro HORIGUCHI wrote:
>> 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())

OK, will replace _guts by _internal.  Looking at the David's comments too.

> About the same function:
>
> Couldn't we get out in the fast path when clauses == NIL?

Actually get_partitions_for_clauses() won't be called at all if that were
true.  I think I should add an Assert that clauses is not 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, &keys);
> + 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, &keys);
>> }

Agreed that your suggested rewrite of that portion of the code is easy to
read, but we cannot return yet in the nkeys == 0 case, as you also said
you found out.  I quote your other reply further below.

> 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.

[ ... ]

On 2017/11/10 14:44, Kyotaro HORIGUCHI wrote:
> At Fri, 10 Nov 2017 14:38:11 +0900, Kyotaro HORIGUCHI wrote:
>
> This is working fine. Sorry for the bogus comment.

I'd almost started looking around if something might be wrong after all. :)

On 2017/11/10 16:07, Kyotaro HORIGUCHI wrote:
> At Fri, 10 Nov 2017 14:44:55 +0900, Kyotaro HORIGUCHI wrote:
>
>>> 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, &keys);
>>>> }
>
> So, the condition (!constfalse && nkeys == 0) cannot return
> there. I'm badly confused by the variable name.

Do you mean by 'constfalse'?

> 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?

OK, will add comments explaining what's going on.


Will post the updated patches after also taking care of David's and Amul's
review comments upthread.

Thanks,
Amit



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

Re: [HACKERS] 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&arch=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]

prefetch" GUC to enable/disable 
prefetching data in postgres_fdw.

This patch should be applied after of partition-wise-agg-v6.tar.gz patch.
With shard example and the following two GUCs set:

shard=# set postgres_fdw.use_prefetch=on;
shard=# set enable_partition_wise_agg=on;
shard=# select sum(product_id) from orders;
   sum
-
 9965891
(1 row)

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)


sum aggregate is calculated in parallel by both servers.

I have not tested it much, it is just prove of concept.

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

270a271,272
> void _PG_init(void);
> 
372a375
> static void prefetch_more_data(ForeignScanState *node);
425a429,430
> static bool fdw_prefetch_data;
> 
1377a1383,1387
> 	create_cursor(node);
> 	if (fdw_prefetch_data)
> 	{
> 		prefetch_more_data(node);
> 	}
2989a3000,3010
> static void
> prefetch_more_data(ForeignScanState *node)
> {
> 	PgFdwScanState *fsstate = (PgFdwScanState *) node->fdw_state;
> 	char		sql[64];
> 	snprintf(sql, sizeof(sql), "FETCH %d FROM c%u",
> 			 fsstate->fetch_size, fsstate->cursor_number);
> 	if (!PQsendQuery(fsstate->conn, sql))
> 		pgfdw_report_error(ERROR, NULL, fsstate->conn, false, sql);
> }
> 
3019c3040,3042
< 		res = pgfdw_exec_query(conn, sql);
---
> 		res = fdw_prefetch_data
> 			? pgfdw_get_result(conn, sql)
> 			: pgfdw_exec_query(conn, sql);
3049d3071
< 
3051a3074,3075
> 		if (!fsstate->eof_reached)
> 			prefetch_more_data(node);
4836a4861,4879
> 
> static bool
> contains_complex_aggregate(Node *node, void *context)
> {
> 	if (node == NULL)
> 		return false;
> 
> 	if (IsA(node, Aggref))
> 	{
> 		Aggref* agg = (Aggref*)node;
> 		return agg->aggtranstype != agg->aggtype;
> 	}
> 
> 	return expression_tree_walker(node,
>   contains_complex_aggregate,
>   context);
> }
> 
> 
4866c4909
< 		if (extra->isPartial)
---
> 		if (extra->isPartial && expression_tree_walker((Node*)extra->pathTarget->exprs, contains_complex_aggregate, NULL))
5190a5234,5246
> }
> 
> void _PG_init(void)
> {
> 	DefineCustomBoolVariable(
> 		"postgres_fdw.use_prefetch",
> 		"Prefetch data from cursor",
> 		NULL,
> 		&fdw_prefetch_data,
> 		false,
> 		PGC_SUSET,
> 		0,
> 		NULL, NULL, NULL);

-- 
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] 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

[HACKERS] ginInsertCleanup called from vacuum could still miss tuples to be deleted

[Masahiko Sawada]

Hi,

Commit e2c79e14 prevented multiple cleanup process for pending list in
GIN index. But I think that there is still possibility that vacuum
could miss tuples to be deleted if someone else is cleaning up the
pending list.

In ginInsertCleanup(), we lock the GIN meta page by LockPage and could
wait for the concurrent cleaning up process if stats == NULL. And the
source code comment says that this happen is when ginINsertCleanup is
called by [auto]vacuum/analyze or gin_clean_pending_list(). I agree
with this behavior. However, looking at the callers the stats is NULL
only either if pending list exceeds to threshold during insertions or
if only analyzing is performed by an autovacum worker or ANALYZE
command. So I think we should inVacuum = (stats != NULL) instead.
Also, we might want autoanalyze and ANALYZE command to wait for
concurrent process as well. Attached patch fixes these two issue. If
this is a bug we should back-patch to 9.6.

void
ginInsertCleanup(GinState *ginstate, bool full_clean,
 bool fill_fsm, IndexBulkDeleteResult *stats)
{

(snip)

boolinVacuum = (stats == NULL);

/*
 * We would like to prevent concurrent cleanup process. For that we will
 * lock metapage in exclusive mode using LockPage() call. Nobody other
 * will use that lock for metapage, so we keep possibility of concurrent
 * insertion into pending list
 */

if (inVacuum)
{
/*
 * We are called from [auto]vacuum/analyze or gin_clean_pending_list()
 * and we would like to wait concurrent cleanup to finish.
 */
LockPage(index, GIN_METAPAGE_BLKNO, ExclusiveLock);
workMemory =
(IsAutoVacuumWorkerProcess() && autovacuum_work_mem != -1) ?
autovacuum_work_mem : maintenance_work_mem;
}
else
{
/*
 * We are called from regular insert and if we see concurrent cleanup
 * just exit in hope that concurrent process will clean up pending
 * list.
 */
if (!ConditionalLockPage(index, GIN_METAPAGE_BLKNO, ExclusiveLock))
return;
workMemory = work_mem;
}

Regards,

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


fix_ginInsertCleanup.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] 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]

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
> assumptions that your code made, such as the assumption that we will
> never have a partial MergeAppend path.
>
> - I changed create_append_path() so that it uses the parallel_aware
> argument as the only determinant of whether the output path is flagged
> as parallel-aware. Your version also considered whether
> parallel_workers > 0, but I think that's not a good idea; the caller
> should pass the correct value for parallel_aware rather than relying
> on this function to fix it.  Possibly you indirectly encountered the
> problem mentioned in the previous point and worked around it like
> this, or maybe there was some other reason, but it doesn't seem to be
> necessary.
>
> - I changed things around to enforce the rule that all partial paths
> added to an appendrel must use the same row count estimate.  (This is
> not a new coding rule, but this patch provides a new way to violate
> it.) I did that by forcing the row-count for any parallel append
> mixing partial and non-partial paths to use the same row count as the
> row already added. I also changed the way the row count is calculated
> in the case where the only parallel append path mixes partial and
> non-partial plans; I think this way is more consistent with what we've
> done elsewhere.  This amounts to the assumption that we're trying to
> estimate the average number of rows per worker rather than the largest
> possible number; I'm not sure what the best thing to do here is in
> theory, but one advantage of this approach is that I think it will
> produce answers closer to the value we get for an all-partial-paths
> append.  That's good, because we don't want the row-count estimate to
> change precipitously based on whether an all-partial-paths append is
> possible.
>
> - I fixed some whitespace problems by running pgindent on various
> files and manually breaking some long lines.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company



-- 
Thanks,
-Amit Khandekar
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] 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 );", context=context@entry=PROCESS_
>> UTILITY_TOPLEVEL,
>> params=params@entry

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

[HACKERS] Row Level Security Bug ?

[Andrea Adami]

Hello,
i have a db with a couple of tables
(enclosed the script to recreate it, please have a look before to proceed)
i enabled the row level security and all seem to work fine

if i do it (connected in as superuser like, usualy, postgres is):

select school, description, example
from schools

i can see all the rows

if i do:

SET ROLE 'manage...@scuola-1.it'

select school, description, example
from school

i see only one row (as expected)

but when i do:

select *
from _rls_test

select *
FROM _rls_test_security_barrier

select *
from _rls_test_with_check_local

select *
from _rls_test_with_check_local_cascade

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.

Thanks to all the spend time to answer me.

here:
https://github.com/scuola247/postgresql
you can have a look at the complete database

Andrea Adami

===
===
===

CREATE DATABASE test
  WITH OWNER = postgres
   ENCODING = 'UTF8'
   TABLESPACE = pg_default
   CONNECTION LIMIT = -1;


CREATE SEQUENCE public.pk_seq
  INCREMENT 1
  MINVALUE 1
  MAXVALUE 9223372036854775807
  START 736220
  CACHE 1;


CREATE TABLE public.schools
(
  school bigint NOT NULL DEFAULT nextval('pk_seq'::regclass), -- Uniquely
identifies the table row
  description character varying(160) NOT NULL, -- Description for the school
  processing_code character varying(160) NOT NULL, -- A code that identify
the school on the government information system
  mnemonic character varying(30) NOT NULL, -- Short description to be use
as code
  example boolean NOT NULL DEFAULT false, -- It indicates that the data
have been inserted to be an example of the use of the data base
  behavior bigint, -- Indicates the subject used for the behavior
  CONSTRAINT schools_pk PRIMARY KEY (school),
  CONSTRAINT schools_uq_description UNIQUE (description),
  CONSTRAINT schools_uq_mnemonic UNIQUE (mnemonic),
  CONSTRAINT schools_uq_processing_code UNIQUE (processing_code, example)
);

-- Index: public.schools_fk_behavior

CREATE INDEX schools_fk_behavior
  ON public.schools
  USING btree
  (behavior);


CREATE TABLE public.usenames_schools
(
  usename_school bigint NOT NULL DEFAULT nextval('pk_seq'::regclass), --
Unique identification code for the row
  usename name NOT NULL, -- The session's usename
  school bigint NOT NULL, -- School enabled for the the usename
  CONSTRAINT usenames_schools_pk PRIMARY KEY (usename_school),
  CONSTRAINT usenames_schools_fk_school FOREIGN KEY (school)
  REFERENCES public.schools (school) MATCH SIMPLE
  ON UPDATE CASCADE ON DELETE CASCADE,
  CONSTRAINT usenames_schools_uq_usename_school UNIQUE (usename, school) --
Foe every usename one school can be enabled only one time
);

-- Index: public.usenames_schools_fx_school

CREATE INDEX usenames_schools_fx_school
  ON public.usenames_schools
  USING btree
  (school);

  CREATE OR REPLACE VIEW public._rls_test AS
 SELECT schools.school,
schools.description,
schools.example
   FROM schools;


CREATE OR REPLACE VIEW public._rls_test_security_barrier WITH
(security_barrier=true) AS
 SELECT schools.school,
schools.description,
schools.example
   FROM schools;

CREATE OR REPLACE VIEW public._rls_test_with_check_local WITH
(check_option=local) AS
 SELECT schools.school,
schools.description,
schools.example
   FROM schools;

CREATE OR REPLACE VIEW public._rls_test_with_check_local_cascade WITH
(check_option=cascaded) AS
 SELECT schools.school,
schools.description,
schools.example
   FROM schools;

-- now same data
-- now same data
-- now same data

INSERT INTO
public.schools(school,description,processing_code,mnemonic,example) VALUES
('289610','Istituto comprensivo "Voyager"','ZZIC1Z','IC
VOYAGER','t');
INSERT INTO
public.schools(school,description,processing_code,mnemonic,example) VALUES
('20','Istituto Tecnico Tecnologico "Leonardo da
Vinci"','ZZITTZ','ITT DAVINCI','t');
INSERT INTO
public.schools(school,description,processing_code,mnemonic,example) VALUES
('10','Istituto comprensivo ''Andromeda''','ZZIC8Z','IC
ANDROMEDA','t');


INSERT INTO public.usenames_schools(usename_school,usename,school) VALUES
('7266330','manage...@scuola-1.it','10');


-- THEN ENABLE ROW LEVEL SECURITY
-- THEN ENABLE ROW LEVEL SECURITY
-- THEN ENABLE ROW LEVEL SECURITY


  ALTER TABLE usenames_schools ENABLE ROW LEVEL SECURITY;

 ALTER TABLE schools ENABLE ROW LEVEL SECURITY;

CREATE POLICY usenames_schools_pl_usename ON usenames_schools TO public
 USING (usename = current_user)
  WITH CHECK (usename = current_user);

CREATE POLICY schools_pl_school ON schools TO

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*)(&isNull) != ((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

[HACKERS] BUG #14897: Segfault on statitics SQL request

[gmail Vladimir Koković]

Hi,

Just to show comment from: gcc/gcc/testsuite/gcc.target/x86_64/abi/defines.h

/* These defines control the building of the list of types to check. There
   is a string identifying the type (with a comma after), a size of the 
type
   (also with a comma and an integer for adding to the total amount of 
types)

   and an alignment of the type (which is currently not really needed since
   the abi specifies that alignof == sizeof for all scalar types).  */


Specifically "the abi specifies that alignof == sizeof for all scalar types"


Vladimir Kokovic

Belgrade, 11.November 2017




--
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]

me, username, hook_at);
+
+		ret = SPI_exec(buf.data, 0);
+		if (ret != SPI_OK_INSERT)
+			elog(ERROR, "SPI_execute failed: error code %d", ret);
+	}
+
+	SPI_finish();
+	PopActiveSnapshot();
+	CommitTransactionCommand();
+}
+
+/* sample session start hook function */
+static void
+sample_session_start_hook()
+{
+	/* Hook just normal backends */
+	if (MyBackendId != InvalidBackendId)
+	{
+		(void) register_session_hook("START");
+
+		if (prev_session_start_hook)
+			prev_session_start_hook();
+	}
+}
+
+/* sample session end hook function */
+static void
+sample_session_end_hook()
+{
+	/* Hook just normal backends */
+	if (MyBackendId != InvalidBackendId)
+	{
+		if (prev_session_end_hook)
+			prev_session_end_hook();
+
+		(void) register_session_hook("END");
+	}
+}
+
+/*
+ * Module Load Callback
+ */
+void
+_PG_init(void)
+{
+	/* Save Hooks for Unload */
+	prev_session_start_hook = session_start_hook;
+	prev_session_end_hook = session_end_hook;
+
+	/* Set New Hooks */
+	session_start_hook = sample_session_start_hook;
+	session_end_hook = sample_session_end_hook;
+
+	/* Load GUCs */
+	DefineCustomStringVariable("test_session_hooks.username",
+			   "Username to register log on session start or end",
+			   NULL,
+			   &session_hook_username,
+			   "postgres",
+			   PGC_SIGHUP,
+			   0, NULL, NULL, NULL);
+}
+
+/*
+ * Module Unload Callback
+ */
+void
+_PG_fini(void)
+{
+	/* Uninstall Hooks */
+	session_start_hook = prev_session_start_hook;
+	session_end_hook = prev_session_end_hook;
+}
diff --git a/src/test/modules/test_session_hooks/test_session_hooks.control b/src/test/modules/test_session_hooks/test_session_hooks.control
new file mode 100644
index 000..7d7ef9f
--- /dev/null
+++ b/src/test/modules/test_session_hooks/test_session_hooks.control
@@ -0,0 +1,3 @@
+comment = 'Test start/end hook session with an extension'
+default_version = '1.0'
+relocatable = true

-- 
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

[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]

   return typtup;
+}

A function as general as typeGet() certainly does not belong in
parse_clause.c in the middle of a long list of temporal functions.
This particular function is also a bad idea in general, because typtup
is only a valid pointer until ReleaseSysCache() is called.  This will
appear to work normally because, normally, no relevant cache
invalidation messages will show up at the wrong time, but if one does
then typtup will be pointing off into outer space.  You can find bugs
like this by testing with CLOBBER_CACHE_ALWAYS defined (warning: this
makes things very slow).  But the general point I want to make here is
actually about inventing your own idioms vs. using the ones we've got
-- if you're going to invent new general-purpose primitives, they need
to go next to the existing functions that do similar things, not in
whatever part of the code you first decided you needed them.

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


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

Re: [HACKERS] 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

[HACKERS] [Patch] Log SSL certificate verification errors

[Graham Leggett]

Hi all,

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.

The SSL test suite runs without regression:

Little-Net:ssl minfrin$ make check
rm -rf '/Users/minfrin/src/postgres/postgres-trunk'/tmp_install
/bin/sh ../../../config/install-sh -c -d 
'/Users/minfrin/src/postgres/postgres-trunk'/tmp_install/log
/Applications/Xcode.app/Contents/Developer/usr/bin/make -C '../../..' 
DESTDIR='/Users/minfrin/src/postgres/postgres-trunk'/tmp_install install 
>'/Users/minfrin/src/postgres/postgres-trunk'/tmp_install/log/install.log 2>&1
rm -rf '/Users/minfrin/src/postgres/postgres-trunk/src/test/ssl'/tmp_check
/bin/sh ../../../config/install-sh -c -d 
'/Users/minfrin/src/postgres/postgres-trunk/src/test/ssl'/tmp_check
cd . && TESTDIR='/Users/minfrin/src/postgres/postgres-trunk/src/test/ssl' 
PATH="/Users/minfrin/src/postgres/postgres-trunk/tmp_install/tmp/postgresql/bin:$PATH"
 
DYLD_LIBRARY_PATH="/Users/minfrin/src/postgres/postgres-trunk/tmp_install/tmp/postgresql/lib"
 PGPORT='65432' 
PG_REGRESS='/Users/minfrin/src/postgres/postgres-trunk/src/test/ssl/../../../src/test/regress/pg_regress'
 /opt/local/bin/prove -I ../../../src/test/perl/ -I .  t/*.pl
t/001_ssltests.pl .. ok 
All tests successful.
Files=1, Tests=40,  6 wallclock secs ( 0.04 usr  0.01 sys +  2.01 cusr  1.21 
csys =  3.27 CPU)
Result: PASS


Index: src/backend/libpq/be-secure-openssl.c
===
--- src/backend/libpq/be-secure-openssl.c   (revision 61109)
+++ src/backend/libpq/be-secure-openssl.c   (working copy)
@@ -999,9 +999,9 @@
 /*
  * Certificate verification callback
  *
- * This callback allows us to log intermediate problems during
- * verification, but for now we'll see if the final error message
- * contains enough information.
+ * There are 50 ways to leave your lover, and 67 ways to fail
+ * certificate verification. Log details of all failed certificate
+ * verification results.
  *
  * This callback also allows us to override the default acceptance
  * criteria (e.g., accepting self-signed or expired certs), but
@@ -1010,6 +1010,28 @@
 static int
 verify_cb(int ok, X509_STORE_CTX *ctx)
 {
+   char *subject, *issuer;
+   X509 *cert;
+   int err, depth;
+
+   if (!ok)
+   {
+   cert = X509_STORE_CTX_get_current_cert(ctx);
+   err = X509_STORE_CTX_get_error(ctx);
+   depth = X509_STORE_CTX_get_error_depth(ctx);
+
+   subject = X509_NAME_oneline(X509_get_subject_name(cert), NULL, 
0);
+   issuer = X509_NAME_oneline(X509_get_issuer_name(cert), NULL, 0);
+
+   ereport(COMMERROR,
+   (errcode(ERRCODE_PROTOCOL_VIOLATION),
+   errmsg("SSL certificate verification result: %s 
(depth %d, subject '%s', issuer '%s')",
+   X509_verify_cert_error_string(err), 
depth, subject, issuer)));
+
+   OPENSSL_free(subject);
+   OPENSSL_free(issuer);
+   }
+
return ok;
 }
 
Index: src/interfaces/libpq/fe-secure-openssl.c
===
--- src/interfaces/libpq/fe-secure-openssl.c(revision 61109)
+++ src/interfaces/libpq/fe-secure-openssl.c(working copy)
@@ -82,6 +82,8 @@
 
 static bool ssl_lib_initialized = false;
 
+static int ssl_ex_data_index;
+
 #ifdef ENABLE_THREAD_SAFETY
 static long ssl_open_connections = 0;
 
@@ -182,6 +184,7 @@
 */
SOCK_ERRNO_SET(0);
ERR_clear_error();
+   resetPQExpBuffer(&conn->errorMessage);
n = SSL_read(conn->ssl, ptr, len);
err = SSL_get_error(conn->ssl, n);
 
@@ -200,7 +203,7 @@
if (n < 0)
{
/* Not supposed to happen, so we don't 
translate the msg */
-   printfPQExpBuffer(&conn->errorMessage,
+   appendPQExpBuffer(&conn->errorMessage,
  "SSL_read 
failed but did not provide error information\n");
/* assume the connection is broken */
result_errno = ECONNRESET;
@@ -224,13 +227,13 @@
result_errno = SOCK_ERRNO;
if (result_errno == EPIPE ||
result_errno == ECONNRESET)
-   printfPQExpBuffer(&conn->errorMessage,
+   appendPQExpBuffer(&conn->errorMess

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]

hot(GetTransactionSnapshot());
+
+	username = GetUserNameFromId(GetUserId(), false);
+
+	/* Register log just for configured username */
+	if (!strcmp(username, session_hook_username))
+	{
+		const char *dbname;
+		int ret;
+		StringInfoData	buf;
+
+		dbname = get_database_name(MyDatabaseId);
+
+		initStringInfo(&buf);
+
+		appendStringInfo(&buf, "INSERT INTO session_hook_log (dbname, username, hook_at) ");
+		appendStringInfo(&buf, "VALUES ('%s', '%s', '%s');",
+			dbname, username, hook_at);
+
+		ret = SPI_exec(buf.data, 0);
+		if (ret != SPI_OK_INSERT)
+			elog(ERROR, "SPI_execute failed: error code %d", ret);
+	}
+
+	SPI_finish();
+	PopActiveSnapshot();
+	CommitTransactionCommand();
+}
+
+/* sample session start hook function */
+static void
+sample_session_start_hook()
+{
+	/* Hook just normal backends */
+	if (MyBackendId != InvalidBackendId)
+	{
+		(void) register_session_hook("START");
+
+		if (prev_session_start_hook)
+			prev_session_start_hook();
+	}
+}
+
+/* sample session end hook function */
+static void
+sample_session_end_hook()
+{
+	/* Hook just normal backends */
+	if (MyBackendId != InvalidBackendId)
+	{
+		if (prev_session_end_hook)
+			prev_session_end_hook();
+
+		(void) register_session_hook("END");
+	}
+}
+
+/*
+ * Module Load Callback
+ */
+void
+_PG_init(void)
+{
+	/* Save Hooks for Unload */
+	prev_session_start_hook = session_start_hook;
+	prev_session_end_hook = session_end_hook;
+
+	/* Set New Hooks */
+	session_start_hook = sample_session_start_hook;
+	session_end_hook = sample_session_end_hook;
+
+	/* Load GUCs */
+	DefineCustomStringVariable("test_session_hooks.username",
+			   "Username to register log on session start or end",
+			   NULL,
+			   &session_hook_username,
+			   "postgres",
+			   PGC_SIGHUP,
+			   0, NULL, NULL, NULL);
+}
+
+/*
+ * Module Unload Callback
+ */
+void
+_PG_fini(void)
+{
+	/* Uninstall Hooks */
+	session_start_hook = prev_session_start_hook;
+	session_end_hook = prev_session_end_hook;
+}
diff --git a/src/test/modules/test_session_hooks/test_session_hooks.control b/src/test/modules/test_session_hooks/test_session_hooks.control
new file mode 100644
index 000..7d7ef9f
--- /dev/null
+++ b/src/test/modules/test_session_hooks/test_session_hooks.control
@@ -0,0 +1,3 @@
+comment = 'Test start/end hook session with an extension'
+default_version = '1.0'
+relocatable = true

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

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

[Bossart, Nathan]

Hi hackers,

The thread for the recent change to allow setting the WAL segment size at
initdb time [0] included a bit of discussion regarding pg_upgrade [1],
where it was suggested that relaxing an error check (presumably in
check_control_data()) might be enough to upgrade servers to a different WAL
segment size.

We've had success with our initial testing of upgrades to larger WAL
segment sizes, including post-upgrade pgbench runs.  Beyond adjusting
check_control_data(), it looks like the 'pg_resetwal -l' call in
copy_xact_xlog_xid() may need to be adjusted to ensure that the WAL
starting address is set to a valid value.

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.

Nathan

[0] 
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=fc49e24fa69a15efacd5b8958115ed9c43c48f9a
[1] 
https://www.postgresql.org/message-id/20160826043926.pwkz45ksxpmfn4g6%40alap3.anarazel.de


-- 
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

[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]

e 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 func_with_bad_set();
 ERROR:  invalid value for parameter "default_text_search_config": "no_such_config"
 reset check_function_bodies;
+-- 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";
+ SERVER_VERSION is consistent 
+--
+ t
+(1 row)
+
diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql
index 23e5029780..af2e353da0 100644
--- a/src/test/regress/sql/guc.sql
+++ b/src/test/regress/sql/guc.sql
@@ -288,3 +288,10 @@ set default_text_search_config = no_such_config;
 select func_with_bad_set();
 
 reset check_function_bodies;
+
+-- 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";

-- 
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/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) &&is_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

[HACKERS] No mention of CREATE STATISTICS in event trigger docs

[David Rowley]

A patch to fix this is attached.

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


event_trigger_statistics_doc.patch
Description: Binary data

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

[HACKERS] Incorrect comment for build_child_join_rel

[Etsuro Fujita]

Hi,

Here is a small patch for $Subject.

Best regards,
Etsuro Fujita
*** a/src/backend/optimizer/util/relnode.c
--- b/src/backend/optimizer/util/relnode.c
***
*** 676,683  build_join_rel(PlannerInfo *root,
   * 'sjinfo': child-join context info
   * 'restrictlist': list of RestrictInfo nodes that apply to this particular
   *pair of joinable relations
!  * 'join_appinfos': list of AppendRelInfo nodes for base child relations
!  *involved in this join
   */
  RelOptInfo *
  build_child_join_rel(PlannerInfo *root, RelOptInfo *outer_rel,
--- 676,682 
   * 'sjinfo': child-join context info
   * 'restrictlist': list of RestrictInfo nodes that apply to this particular
   *pair of joinable relations
!  * 'jointype' is the join type (inner, left, full, etc)
   */
  RelOptInfo *
  build_child_join_rel(PlannerInfo *root, RelOptInfo *outer_rel,

-- 
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

[HACKERS] pg audit requirements

[Pavel Stehule]

Hi

I am sending some notes, experience about usage of pgAudit.

pgAudit provides basic functionality and usually is good enough. But it is
not good enough for some applications in financial services.

The requirements:

1. structured output - attached query is not good enough - column name,
table name, schema, database, role should be separated

2. separated log (log file) with guaranteed write - fsync after every line
means significant performance issue, but fsync every 1sec (or defined
interval) is acceptable

3. security issues - not enough access rights to database object should be
processed and logged in audit log too.

Regards

Pavel

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.
>