Re: [HACKERS] Jsonb transform for pl/python

[Aleksander Alekseev]

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

Hello Anthony,

Great job!

I decided to take a closer look on your patch. Here are some defects I 
discovered.

> +   Additional extensions are available that implement transforms for
> +   the jsonb type for the language PL/Python.  The
> +   extensions for PL/Perl are called

1. The part regarding PL/Perl is obviously from another patch.

2. jsonb_plpython2u and jsonb_plpythonu are marked as relocatable, while 
jsonb_plpython3u is not. Is it a mistake? Anyway if an extension is relocatable
there should be a test that checks this.

3. Not all json types are test-covered. Tests for 'true' :: jsonb, '3.14' :: 
jsonb and 'null' :: jsonb are missing.

4. jsonb_plpython.c:133 - "Iterate trhrough Jsonb object." Typo, it should be 
"through" or probably even "over".

5. It looks like you've implemented transform in two directions Python <-> 
JSONB, however I see tests only for Python <- JSONB case.

6. Tests passed on Python 2.7.14 but failed on 3.6.2:

>  CREATE EXTENSION jsonb_plpython3u CASCADE;
> + ERROR:  could not access file "$libdir/jsonb_plpython3": No such file or
> directory

module_pathname in jsonb_plpython3u.control should be $libdir/jsonb_plpython3u,
not $libdir/jsonb_plpython3.

Tested on Arch Linux x64, GCC 7.2.0.

The new status of this patch is: Waiting on Author

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

[HACKERS] Re: [BUGS] 10.0: Logical replication doesn't execute BEFORE UPDATE OF trigger

[Aleksander Alekseev]

Hi Petr,

> let me start by saying that my view is that this is simply a
> documentation bug. Meaning that I didn't document that it does not work,
> but I also never intended it to work. Main reason is that we can't
> support the semantics of "UPDATE OF" correctly (see bellow). And I think
> it's better to not support something at all rather than making it behave
> differently in different cases.
> 
> Now about the proposed patch, I don't think this is correct way to
> support this as it will only work when either PRIMARY KEY column was
> changed or when REPLICA IDENTITY is set to FULL for the table. And even
> then it will have very different semantics from how it works when the
> row is updated by SQL statement (all non-toasted columns will be
> reported as changed regardless of actually being changed or not).
> 
> The more proper way to do this would be to run data comparison of the
> new tuple and the existing tuple values which a) will have different
> behavior than normal "UPDATE OF" triggers have because we still can't
> tell what columns were mentioned in the original query and b) will not
> exactly help performance (and perhaps c) one can write the trigger to
> behave this way already without impacting all other use-cases).

I personally think that solution proposed by Masahiko is much better
than current behavior. We could document current limitations you've
mentioned and probably add a corresponding warning during execution of
ALTER TABLE ... ENABLE REPLICA TRIGGER. I don't insist on this
particular approach though.

What I really don't like is that PostgreSQL allows to create something
that supposedly should work but in fact doesn't. Such behavior is
obviously a bug. So as an alternative we could just return an error in
response to ALTER TABLE ... ENABLE REPLICA TRIGGER query for triggers
that we know will never be executed.

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

[HACKERS] Re: [BUGS] 10.0: Logical replication doesn't execute BEFORE UPDATE OF trigger

[Aleksander Alekseev]

Hi Masahiko,

> > I think the cause of this issue is that the apply worker doesn't set
> > updatedCols of RangeTblEntry when applying updates. So TriggerEnabled
> > always ends up with false. I'll make a patch and submit.
> >
> 
> Attached patch store the updated columns bitmap set to RangeTblEntry.
> In my environment this bug seems to be fixed by the patch.

Thanks a lot for a quick response. I can confirm that your patch fixes
the issue and passes all tests. Hopefully someone will merge it shortly.

Here is another patch from me. It adds a corresponding TAP test. Before
applying your patch:

```
t/001_rep_changes.pl .. ok
t/002_types.pl  ok   
t/003_constraints.pl .. 1/5
#   Failed test 'check replica trigger with specified list of affected columns 
applied on subscriber'
#   at t/003_constraints.pl line 151.
#  got: 'k2|v1'
# expected: 'k2|v1
# triggered|true'
# Looks like you failed 1 test of 5.
t/003_constraints.pl .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/5 subtests
t/004_sync.pl . ok
t/005_encoding.pl . ok
t/006_rewrite.pl .. ok
t/007_ddl.pl .. ok
```

After:

```
t/001_rep_changes.pl .. ok
t/002_types.pl  ok
t/003_constraints.pl .. ok
t/004_sync.pl . ok
t/005_encoding.pl . ok
t/006_rewrite.pl .. ok
t/007_ddl.pl .. ok
All tests successful.
```

-- 
Best regards,
Aleksander Alekseev
diff --git a/src/test/subscription/t/003_constraints.pl b/src/test/subscription/t/003_constraints.pl
index 06863aef84..f1fc5ae863 100644
--- a/src/test/subscription/t/003_constraints.pl
+++ b/src/test/subscription/t/003_constraints.pl
@@ -3,7 +3,7 @@ use strict;
 use warnings;
 use PostgresNode;
 use TestLib;
-use Test::More tests => 4;
+use Test::More tests => 5;
 
 # Initialize publisher node
 my $node_publisher = get_new_node('publisher');
@@ -21,6 +21,9 @@ $node_publisher->safe_psql('postgres',
 $node_publisher->safe_psql('postgres',
 "CREATE TABLE tab_fk_ref (id int PRIMARY KEY, bid int REFERENCES tab_fk (bid));"
 );
+$node_publisher->safe_psql('postgres',
+"CREATE TABLE tab_upd_tst (k TEXT PRIMARY KEY, v TEXT);"
+);
 
 # Setup structure on subscriber
 $node_subscriber->safe_psql('postgres',
@@ -28,6 +31,9 @@ $node_subscriber->safe_psql('postgres',
 $node_subscriber->safe_psql('postgres',
 "CREATE TABLE tab_fk_ref (id int PRIMARY KEY, bid int REFERENCES tab_fk (bid));"
 );
+$node_subscriber->safe_psql('postgres',
+"CREATE TABLE tab_upd_tst (k TEXT PRIMARY KEY, v TEXT);"
+);
 
 # Setup logical replication
 my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
@@ -112,5 +118,38 @@ $result = $node_subscriber->safe_psql('postgres',
 	"SELECT count(*), min(bid), max(bid) FROM tab_fk_ref;");
 is($result, qq(2|1|2), 'check replica trigger applied on subscriber');
 
+# Add replica trigger with specified list of affected columns
+$node_subscriber->safe_psql(
+	'postgres', qq{
+CREATE OR REPLACE FUNCTION upd_fn()
+RETURNS trigger AS \$\$
+BEGIN
+INSERT INTO tab_upd_tst VALUES ('triggered', 'true');
+RETURN NEW;
+END
+\$\$ LANGUAGE plpgsql;
+
+CREATE TRIGGER upd_trg
+BEFORE UPDATE OF k ON tab_upd_tst
+FOR EACH ROW EXECUTE PROCEDURE upd_fn();
+
+ALTER TABLE tab_upd_tst ENABLE REPLICA TRIGGER upd_trg;
+});
+
+# Insert data
+$node_publisher->safe_psql('postgres',
+	"INSERT INTO tab_upd_tst (k, v) VALUES ('k1', 'v1');");
+$node_publisher->safe_psql('postgres',
+	"UPDATE tab_upd_tst SET k = 'k2' WHERE k = 'k1';");
+
+$node_publisher->poll_query_until('postgres', $caughtup_query)
+  or die "Timed out while waiting for subscriber to catch up";
+
+# Trigger should be executed
+$result =
+  $node_subscriber->safe_psql('postgres', "SELECT k, v FROM tab_upd_tst ORDER BY k");
+is( $result, qq(k2|v1
+triggered|true), 'check replica trigger with specified list of affected columns applied on subscriber');
+
 $node_subscriber->stop('fast');
 $node_publisher->stop('fast');


signature.asc
Description: PGP signature

[HACKERS] 10.0: Logical replication doesn't execute BEFORE UPDATE OF trigger

[Aleksander Alekseev]

Hi hackers,

I've found something that looks like a bug.

Steps to reproduce
--

There are 3 instances of PostgreSQL 10.0 - inst1, inst2 and inst3. There
is a table `test` on every instance:

```
CREATE TABLE test(k TEXT PRIMARY KEY, v TEXT);
```

Both inst1 and inst2 have `allpub` publication:

```
CREATE PUBLICATION allpub FOR ALL TABLES;
```

... and inst3 is subscribed for both publications:

```
CREATE SUBSCRIPTION allsub1
  CONNECTION 'host=10.128.0.16 user=eax dbname=eax'
  PUBLICATION allpub;

CREATE SUBSCRIPTION allsub2
  CONNECTION 'host=10.128.0.26 user=eax dbname=eax'
  PUBLICATION allpub;
```

So basically it's two masters, one replica configuration. To resolve
insert/update conflicts I've created the following triggers on inst3:

```
CREATE OR REPLACE FUNCTION test_before_insert()
RETURNS trigger AS $$
BEGIN

RAISE NOTICE 'test_before_insert trigger executed';

IF EXISTS (SELECT 1 FROM test where k = new.k) THEN
   RAISE NOTICE 'test_before_insert trigger - merging data';
   UPDATE test SET v = v || ';' || new.v WHERE k = new.k;
   RETURN NULL;
END IF;

RETURN NEW;

END
$$ LANGUAGE plpgsql;


CREATE OR REPLACE FUNCTION test_before_update()
RETURNS trigger AS $$
BEGIN

RAISE NOTICE 'test_before_update trigger executed';

IF EXISTS (SELECT 1 FROM test where k = new.k) THEN
   RAISE NOTICE 'test_before_update trigger - merging data';
   UPDATE test SET v = v || ';' || new.v WHERE k = new.k;
   DELETE FROM test where k = old.k;
   RETURN NULL;
END IF;

RETURN NEW;

END
$$ LANGUAGE plpgsql;

create trigger test_before_insert_trigger
before insert on test
for each row execute procedure test_before_insert();

create trigger test_before_update_trigger
before update of k on test
for each row execute procedure test_before_update();

ALTER TABLE test ENABLE REPLICA TRIGGER test_before_insert_trigger;
ALTER TABLE test ENABLE REPLICA TRIGGER test_before_update_trigger;
```

The INSERT trigger works just as expected, however the UPDATE trigger
doesn't. On inst1:

```
insert into test values ('k1', 'v1');
```

In inst2:

```
insert into test values ('k4', 'v4');
update test set k = 'k1' where k = 'k4';
```

Now on inst3:

```
select * from test;
```

Expected result
---

Rows are merged to:

```
 k  |   v   
+---
 k1 | v1;v4
```

This is what would happen if all insert/update queries would have been
executed on one instance.

Actual result
-

Replication fails, log contains:

```
[3227] ERROR:  duplicate key value violates unique constraint "test_pkey"
[3227] DETAIL:  Key (k)=(k1) already exists.
[3176] LOG:  worker process: logical replication worker for subscription 16402 
(PID 3227) exited with exit code 1
```

What do you think?

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Automatic testing of patches in commit fest

[Aleksander Alekseev]

Hi Thomas,

> 1.  I didn't have --enable-cassert enabled before.  Oops.
> 2.  It'll now dump a gdb backtrace for any core files found after a
> check-world failure (if you can find your way to the build log...).
> Thanks to Andres for the GDB scripting for this!
> 3.  It'll now push gcov results to codecov.io -- see link at top of
> page.  Thanks again to Andres for this idea.
> 4.  It now builds a little bit faster due to -j4 (Travis CI VMs have 2
> virtual cores) and .proverc -j3.  (So far one entry now fails in TAP
> tests with that setting, will wait longer before drawing any
> conclusions about that.)

Wow. Well done!

> LLVM sanitizer stuff

In my experience it doesn't work well with PostgreSQL code, way to many
false positives. For more details see this discussion [1].

> Valgrind

That would be great. Please note that Valgrind generates false reports
regarding memory leaks in PostgreSQL so you should use --leak-check=no.
Also USE_VALGRIND should be defined in src/include/pg_config_manual.h
before building PostgreSQL. Here [2] is a script I'm using.

> Coverity

I believe it would be not easy to integrate with the web-version of
Coverity. On the other hand Clang Static Analyzer often finds real
defects and it's very easy to integrate with it. Here is my script [3].

> more compilers, 

In my experience trying to compile a patch on FreeBSD with Clang often
helps to find some sort of defect. Same regarding trying different
architectures, e.g. x64, x86, arm.

[1]: https://www.postgresql.org/message-id/20160321130850.6ed6f598%40fujitsu
[2]: https://github.com/afiskon/pgscripts/blob/master/valgrind.sh
[3]: https://github.com/afiskon/pgscripts/blob/master/static-analysis.sh

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Patches that don't apply or don't compile: 2017-09-12

[Aleksander Alekseev]

Hi Martin,

> > === Build Failed: 7 ===
> > Title: Fix the optimization to skip WAL-logging on table created in same 
> > transaction
> > Author: Martijn van Oosterhout <klep...@svana.org>
> > URL: https://commitfest.postgresql.org/14/528/
> 
> I'm not the author of this patch, and the page doesn't say so.
> Something wrong with your script?

It's a bug. The authors were determined by the senders of the first
email in the corresponding thread. This is because I needed email list
to CC the authors but the commitfest application doesn't show emails.

Sorry for the disturbance.

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Patches that don't apply or don't compile: 2017-09-12

[Aleksander Alekseev]

Hi Tomas,

I appreciate your feedback, although it doesn't seem to be completely
fair. Particularly:

> You gave everyone about 4 hours to object

This is not quite accurate since my proposal was sent 2017-09-11
09:41:32 and this thread started - 2017-09-12 14:14:55.

> You just changed the status of 10-15% open patches. I'd expect
> things like this to be consulted with the CF manager, yet I don't see
> any comments from Daniel.

Agree, that was clearly a mistake, I had to add Daniel to CC. Sorry I
didn't do that. I've returned all affected patches back to "Needs
Review". On the bright side while doing this I've noticed that many
patches were already updated by their authors.

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] WIP patch: distinguish selectivity of < from <= and > from >=

[Aleksander Alekseev]

Hi Tom,

> Thanks for reviewing!  I assume this is against the prior version of the
> patch, though, not the one I just posted with updates for contrib.
> Do you want to look at those?
> 
>   regards, tom lane

No, I reviewed the latest v4 patch right after you submitted it.

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] WIP patch: distinguish selectivity of < from <= and > from >=

[Aleksander Alekseev]

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

Also I didn't manage to find any typos or obvious mistakes in the code.

The new status of this patch is: Ready for Committer

-- 
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] Patches that don't apply or don't compile: 2017-09-12

[Aleksander Alekseev]

Hi Andreas,

On 09/12/2017 04:14 PM, Aleksander Alekseev wrote:
> > Title: Foreign Key Arrays
> > Author: Mark Rofail<markm.rof...@gmail.com>
> > URL:https://commitfest.postgresql.org/14/1252/
> 
> I am currently reviewing this one and it applies, compiles, and passes the
> test suite. It could be the compilation warnings which makes the system
> think it failed, but I could not find the log of the failed build.

Thanks for your feedback. I'm changing the status of this patch back to
"Needs Review" and adding it to the exclude list until we will figure
out what went wrong.

> We want to be welcoming to new contributors so until we have a reliable CI
> server which can provide easy to read build logs I am against changing the
> status of any patches solely based on the result of the CI server. I think
> it should be used as a complimentary tool until the community deems it to be
> good enough.

Agree, especially regarding build logs. All of this currently is only an
experiment. For some reason I got a weird feeling that at this time it
will be not quite successful one. If there will be too many false
positives I'll just return the patches back to "Needs Review" as it was
before. But I strongly believe that after a few iterations we will find
a solution that will be good enough and this slight inconveniences will
worth it in a long run.

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

[HACKERS] Patches that don't apply or don't compile: 2017-09-12

[Aleksander Alekseev]

//commitfest.postgresql.org/14/775/


=== Build Failed: 7 ===
Title: Fix the optimization to skip WAL-logging on table created in same 
transaction
Author: Martijn van Oosterhout <klep...@svana.org>
URL: https://commitfest.postgresql.org/14/528/

Title: Foreign Key Arrays
Author: Mark Rofail <markm.rof...@gmail.com>
URL: https://commitfest.postgresql.org/14/1252/

Title: Generic type subscripting
Author: Dmitry Dolgov <9erthali...@gmail.com>
URL: https://commitfest.postgresql.org/14/1062/

Title: new plpgsql extra_checks 
Author: Pavel Stehule <pavel.steh...@gmail.com>
URL: https://commitfest.postgresql.org/14/962/

Title: Replication status in logical replication
Author: Masahiko Sawada <sawada.m...@gmail.com>
URL: https://commitfest.postgresql.org/14/1113/

Title: Restricting maximum keep segments by repslots
Author: Kyotaro HORIGUCHI <horiguchi.kyot...@lab.ntt.co.jp>
URL: https://commitfest.postgresql.org/14/1260/

Title: Temporal query processing with range types
Author: Anton Dignös <dign...@inf.unibz.it>
URL: https://commitfest.postgresql.org/14/839/

Needs Review Total: 120
Failed Total: 32 (26.67 %)



As always, any feedback is very welcomed!

[1]: http://commitfest.cputube.org/
[2]: 
https://postgr.es/m/CAB7nPqSrHF%2BkNQ6Eq2uy91RcysoCzQr1AjOjkuCn9jvMdJZ6Fw%40mail.gmail.com
[3]: https://github.com/afiskon/py-autoreviewer

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Automatic testing of patches in commit fest

[Aleksander Alekseev]

Hi Tomas,

> > I hope this observation will change your mind :)
> > 
> 
> Not sure. But I'm mostly just a passenger here, not the driver.

After working on this script for some time I got second thoughts
regarding this idea as well. The reason for this is that the script is
just a bunch of regular expressions that parse HTML. It works today but
doesn't look like something I'm willing to maintain in the long run.

I've ended up with this script [1]. It just generates a list of patches
that are in "Needs Review" status but don't apply or don't compile. Here
is the current list:

```
=== Apply Failed: 29 ===
https://commitfest.postgresql.org/14/1178/ (64-bit transaction identifiers)
https://commitfest.postgresql.org/14/1090/ (Add and report the new 
in_hot_standby GUC pseudo-variable.)
https://commitfest.postgresql.org/14/1139/ (allow mix of composite and scalar 
variables in target list)
https://commitfest.postgresql.org/14/1131/ (Allow users to specify multiple 
tables in VACUUM commands)
https://commitfest.postgresql.org/14/1284/ (Controlling toast_tuple_target to 
tune rows 2kB)
https://commitfest.postgresql.org/14/1001/ (Convert join OR clauses into UNION 
queries)
https://commitfest.postgresql.org/14/1272/ (faster partition pruning in planner)
https://commitfest.postgresql.org/14/1259/ (Fix race condition between SELECT 
and ALTER TABLE NO INHERIT)
https://commitfest.postgresql.org/14/1156/ (Gather speed-up)
https://commitfest.postgresql.org/14/1271/ (generated columns)
https://commitfest.postgresql.org/14/1059/ (Hash partitioning)
https://commitfest.postgresql.org/14/1138/ (Improve compactify_tuples and 
PageRepairFragmentation)
https://commitfest.postgresql.org/14/1136/ (Improve eval_const_expressions)
https://commitfest.postgresql.org/14/1124/ (Incremental sort)
https://commitfest.postgresql.org/14/1228/ (libpq: Support for Apple Secure 
Transport SSL library)
https://commitfest.postgresql.org/14/1238/ (multivariate MCV lists and 
histograms)
https://commitfest.postgresql.org/14/1202/ (New function for tsquery creation)
https://commitfest.postgresql.org/14/1250/ (Partition-wise aggregation/grouping)
https://commitfest.postgresql.org/14/1125/ (pg_basebackup has stricter 
tablespace rules than the server)
https://commitfest.postgresql.org/14/1183/ (Predicate locking in hash index)
https://commitfest.postgresql.org/14/1106/ (Range Merge Join)
https://commitfest.postgresql.org/14/1267/ (Retire polyphase merge)
https://commitfest.postgresql.org/14/1248/ (Subscription code improvements)
https://commitfest.postgresql.org/14/1235/ (Support arrays over domain types)
https://commitfest.postgresql.org/14/1148/ (Support 
target_session_attrs=read-only and eliminate the added round-trip to detect 
session status.)
https://commitfest.postgresql.org/14/1242/ (taking stdbool.h into use)
https://commitfest.postgresql.org/14/1130/ (Test coverage for FDW HANDLER DDL.)
https://commitfest.postgresql.org/14/1023/ (UPDATE of partition key)
https://commitfest.postgresql.org/14/775/ (Write Amplification Reduction Method 
(WARM))

=== Build Failed: 7 ===
https://commitfest.postgresql.org/14/528/ (Fix the optimization to skip 
WAL-logging on table created in same transaction)
https://commitfest.postgresql.org/14/1252/ (Foreign Key Arrays)
https://commitfest.postgresql.org/14/1062/ (Generic type subscripting)
https://commitfest.postgresql.org/14/962/ (new plpgsql extra_checks )
https://commitfest.postgresql.org/14/1113/ (Replication status in logical 
replication)
https://commitfest.postgresql.org/14/1260/ (Restricting maximum keep segments 
by repslots)
https://commitfest.postgresql.org/14/839/ (Temporal query processing with range 
types)
```

Unless there are any objections I'm going to give these patches "Waiting
on Author" status today (before doing this I'll re-run the script to
make sure that the list is up-to-date). I'm also going to write one more
email with CC to the authors of these patches to let them know that the
status of their patch has changed.

[1]: https://github.com/afiskon/py-autoreviewer

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Automatic testing of patches in commit fest

[Aleksander Alekseev]

Hi Tomas,

> > Unless there are any objections to give this idea a try I'm willing to
> > write and host a corresponding script.
> > 
> That won't work until (2) is reliable enough. There are patches (for
> example my "multivariate MCV lists and histograms") which fails to apply
> only because the tool picks the wrong patch. Possibly because it does
> not recognize compressed patches, or something.

Agree. However we could simply add an "Enable autotests" checkbox to the
commitfest application. In fact we could even left the commitfest
application as is and provide corresponding checkboxes in the web
interface of the "autoreviewer" application. Naturally every
automatically generated code review will include a link that disables
autotests for this particular commitfest entry.

I hope this observation will change your mind :)

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Automatic testing of patches in commit fest

[Aleksander Alekseev]

Hi Thomas,

Great job!

Here is a crazy idea. What if we write a script that would automatically
return the patches that:

1) Are not in "Waiting on Author" status
2) Don't apply OR don't pass `make installcheck-world`

... to the "Waiting on Author" status and describe the problem through
the "Add review" form on commitfest.postgresql.org? This will save a lot
of time to the reviewers. Naturally nobody wants to spam pgsql-hackers@
with automatic messages to often. I believe that sending such messages
once a day would be enough.

Unless there are any objections to give this idea a try I'm willing to
write and host a corresponding script.

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Red-black trees: why would anyone want preorder or postorder traversal?

[Aleksander Alekseev]

Hi Tom,

> In short, therefore, I propose we rip out the DirectWalk and InvertedWalk
> options along with their support code, and then drop the portions of
> test_rbtree that are needed to exercise them.  Any objections?

Doesn't sound like something that will be used any time soon. When and
if it will happen nothing prevents us from adding this code back. So
it's +1.

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] [PATCH] Improve geometric types

[Aleksander Alekseev]

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

LGTM.

The new status of this patch is: Ready for Committer

-- 
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] Improve geometric types

[Aleksander Alekseev]

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

PostgreSQL fails with SIGSEGV during `make check-world`.

Backtrace: http://afiskon.ru/s/d4/f3dc17838a_sigsegv.txt
regression.diffs (not very useful): 
http://afiskon.ru/s/ac/ac5294656c_regression.diffs.txt
regression.out: http://afiskon.ru/s/70/39d872e2b8_regression.out.txt
How to reproduce: https://github.com/afiskon/pgscripts/blob/master/full-build.sh

The environment is Arch Linux x64, gcc 7.1.1

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] [PATCH] Improve geometric types

[Aleksander Alekseev]

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

Hi Emre,

I'm afraid these patches conflict with current master branch.

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] [PATCH] Off-by-one error in logical slot resource retention

[Aleksander Alekseev]

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

Hi Craig,

I'm afraid patches 0002 and 0003 don't apply anymore. Could you please resolve 
the conflicts?

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] WIP Patch: Precalculate stable functions, infrastructure v1

[Aleksander Alekseev]

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

Hi Marina,

I'm sorry to inform you that the v5 path set become a little outdated:

```
$ git apply v5-0002-Precalculate-stable-functions-planning-and-execut.patch
error: patch failed: src/pl/plpgsql/src/pl_exec.c:6471
error: src/pl/plpgsql/src/pl_exec.c: patch does not apply
```

If it's not too much trouble could you please fix the conflicts with the 
current master branch?

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] proposal: psql command \graw

[Aleksander Alekseev]

Hi Pavel,

> I am thinking about printing graphs in psql (mainly some histograms). I
> found so gnuplot is able do very good graphs in console. The one issue is
> user friendly (with less steps) generating data in good format for this
> application.
> 
> One my idea is introduction new simple output format and execution command
> with result in this format.
> 
> It should work something like
> 
> \setenv GNUPLOT_OPTION '..'
> 
> SELECT * FROM data
> 
> \graw | gnuplot ...
> 
> It can be used for any other applications R, ggplot, ..
> 
> Ideas, comments?

Sounds cool. On the other hand, I think it's kind of too domain specific
task. So I wonder whether it could be generalized somehow so anyone
could write an extension that would export data in any format in a
friendly way.

For instance:

create extension export_to_gnuplot;
select * from data
\export_to_gnuplot | gnuplot ...

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] SCRAM salt length

[Aleksander Alekseev]

He Peter,

> The SCRAM salt length is currently set as
> 
> /* length of salt when generating new verifiers */
> #define SCRAM_DEFAULT_SALT_LEN 12
> 
> without further comment.
> 
> I suspect that this length was chosen based on the example in RFC 5802
> (SCRAM-SHA-1) section 5.  But the analogous example in RFC 7677
> (SCRAM-SHA-256) section 3 uses a length of 16.  Should we use that instead?

Maybe this length was chosen just because it becomes a 16-characters
string after base64encode. If I understand correctly RFC 5802 and RFC
7677 don't say much about the required or recommended length of the
salt.

I personally believe that 2^96 of possible salts is consistent with both
RFCs and should be enough in practice.

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Regressions failures with libxml2 on ArchLinux

[Aleksander Alekseev]

Hi Michael,

> I can confirm that I see the same errors on Arch Linux with latest
> updates when PostgreSQL is compiled with --with-libxml and/or
> --with-libxslt. I submitted a few details on bugs.archlinux.org [1]
> since probably not all Arch Linux maintainers know how to reproduce an
> issue.
> 
> [1]: https://bugs.archlinux.org/task/55134

TWIMC I've also described a workaround there.

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Regressions failures with libxml2 on ArchLinux

[Aleksander Alekseev]

Hi Michael,

> It's been one month since I have done some serious development with
> Archlinux (I was abroad and away from the laptop dedicated to that),
> and surprise, I can see failures in the PG regression tests, like the
> following short extract (result compared to expected/xml.out):

I can confirm that I see the same errors on Arch Linux with latest
updates when PostgreSQL is compiled with --with-libxml and/or
--with-libxslt. I submitted a few details on bugs.archlinux.org [1]
since probably not all Arch Linux maintainers know how to reproduce an
issue.

[1]: https://bugs.archlinux.org/task/55134

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Funny WAL corruption issue

[Aleksander Alekseev]

Hi Chris,

> I ran into a funny situation today regarding PostgreSQL replication and
> wal corruption and wanted to go over what I think happened and what I
> wonder about as a possible solution.

Sad story. Unfortunately I have no idea what could be a reason nor can I
suggest a good way to find it unless there is an already know sequence
of steps that reproduces an issue.

I just wanted to point out that a hardware issue or third party software
issues (bugs in FS, software RAID, ...) could not be fully excluded from
the list of suspects. According to the talk by Christophe Pettus [1]
it's not that uncommon as most people think.

If the issue reproduces from time to time on one replica and doesn't on
the second identical replica there is a good chance that you've faced a
hardware issue. Another thing that is worth checking is a SMART status
of the hard drive.

[1]: 
http://www.pgcon.org/2017/schedule/attachments/453_corruption-pgcon-2017.pdf

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Cache lookup errors with functions manipulation object addresses

[Aleksander Alekseev]

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

I believe this patch is "Ready for Committer".

The new status of this patch is: Ready for Committer

-- 
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] Red-Black tree traversal tests

[Aleksander Alekseev]

Hi Victor,

> I forgot to attach the patch. Sorry.
> Here it is.

I would say that this patch is in a pretty good shape now. And I see a
99% code coverage of rbtree.c. Let's see what committers think.

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Red-Black tree traversal tests

[Aleksander Alekseev]

Hi Victor,

> If it's not too much trouble perhaps you could write a few more test so
> we would have 100% test coverage for rbtree, could modify it safely and
> be sure that it actually works when someone will need the rest of its
> functionality?

Also I would recommend to add your patch to the nearest commitfest [1].
Otherwise there is a good chance that everyone will forget about it
quite soon.

[1]: https://commitfest.postgresql.org/14/

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Red-Black tree traversal tests

[Aleksander Alekseev]

Hi Victor,

> Postgres now has its own red-black tree implementation. This tree has 4
> types of traversals. In the attachment, you can find module test that
> checks the correctness of tree traversal strategies.
> 
> I hope that someone can find it useful.

Great job! However, according to lcov report, some procedures declared
in rbtree.c are still not test covered even with your patch,
particularly:

* rb_find
* rb_leftmost
* rb_delete + dependencies (rb_delete_fixup, etc)

You can generate a corresponding report using this script [1].

I must say, I was a little surprised that rb_find is not used anywhere
in PostgreSQL code. Turned out that rbtree currently is used only by GIN
and it uses a small subset of all procedures. 

If it's not too much trouble perhaps you could write a few more test so
we would have 100% test coverage for rbtree, could modify it safely and
be sure that it actually works when someone will need the rest of its
functionality?

[1]: https://github.com/afiskon/pgscripts/blob/master/code-coverage.sh


-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] A couple of postgresql.conf.sample discrepancies

[Aleksander Alekseev]

Hi Thomas,

> Here's a script that reminds you about GUCs you forgot to put in
> postgresql.conf.sample.  It probably needs some work.  Does this
> already happen somewhere else?  I guess not, because it found two
> discrepancies:
> 
> $ ./src/tools/check_sample_config.pl
> enable_gathermerge appears in guc.c but not in postgresql.conf.sample
> trace_recovery_messages appears in guc.c but not in postgresql.conf.sample

I like the idea. However maybe it worth considering to turn it into a
TAP test? Otherwise there is a good chance everybody will forget to run
it. For similar reason I would advise to add this patch to the next
commitfest.

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Restrictions of logical replication

[Aleksander Alekseev]

Hi Tatsuo,

On Fri, Jun 16, 2017 at 04:00:56PM +0900, Tatsuo Ishii wrote:
> Maybe I am missing something, but I could not find any description
> that logical replication does not support large objects and TRUNCATE
> in the doc.  Do we want to add them to the doc as the restrictions of
> the logical replication?

I knew about TRUNCATE and it most definitely should be documented if
it's not yet. And what about large objects?

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] WIP: Data at rest encryption

[Aleksander Alekseev]

> > While I agree that configuring full disk encryption is not technically
> > difficult, it requires much more privileged access to the system and
> > basically requires the support of a system administrator. In addition,
> > if a volume is not available for encryption, PostgreSQL support for
> > encryption would still allow for its data to be encrypted and as others
> > have mentioned can be enabled by the DBA alone.
> 
> Frankly I'm having difficulties imagining when it could be a real
> problem. It doesn't seem to be such a burden to ask a colleague for
> assistance in case you don't have sufficient permissions to do
> something. And I got a strong feeling that solving bureaucracy issues of
> specific organizations by changing PostgreSQL core in very invasive way
> (keeping in mind testing, maintaining, etc) is misguided.

In the same time implementing a plugable storage API and then implementing
encrypted / compressed / whatever storage in a standalone extension using
this API seems to be a reasonable thing to do. 

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] WIP: Data at rest encryption

[Aleksander Alekseev]

Hi Kenneth,

> > > File system encryption already exists and is well-tested.  I don't see
> > > any big advantages in re-implementing all of this one level up.  You
> > > would have to touch every single place in PostgreSQL backend and tool
> > > code where a file is being read or written.  Yikes.
> > 
> > I appreciate your work, but unfortunately I must agree with Peter.
> > 
> > On Linux you can configure the full disc encryption using LUKS /
> > dm-crypt in like 5 minutes [1]. On FreeBSD you can do the same using
> > geli [2]. In my personal opinion PostgreSQL is already complicated
> > enough. A few companies that hired system administrators that are too
> > lazy to read two or three man pages is not a reason to re-implement file
> > system encryption (or compression, or mirroring if that matters) in any
> > open source RDBMS.
> 
> While I agree that configuring full disk encryption is not technically
> difficult, it requires much more privileged access to the system and
> basically requires the support of a system administrator. In addition,
> if a volume is not available for encryption, PostgreSQL support for
> encryption would still allow for its data to be encrypted and as others
> have mentioned can be enabled by the DBA alone.

Frankly I'm having difficulties imagining when it could be a real
problem. It doesn't seem to be such a burden to ask a colleague for
assistance in case you don't have sufficient permissions to do
something. And I got a strong feeling that solving bureaucracy issues of
specific organizations by changing PostgreSQL core in very invasive way
(keeping in mind testing, maintaining, etc) is misguided.

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] WIP: Data at rest encryption

[Aleksander Alekseev]

Hi Ants,

On Tue, Jun 13, 2017 at 09:07:49AM -0400, Peter Eisentraut wrote:
> On 6/12/17 17:11, Ants Aasma wrote:
> > I'm curious if the community thinks this is a feature worth having?
> > Even considering that security experts would classify this kind of
> > encryption as a checkbox feature.
> 
> File system encryption already exists and is well-tested.  I don't see
> any big advantages in re-implementing all of this one level up.  You
> would have to touch every single place in PostgreSQL backend and tool
> code where a file is being read or written.  Yikes.

I appreciate your work, but unfortunately I must agree with Peter.

On Linux you can configure the full disc encryption using LUKS /
dm-crypt in like 5 minutes [1]. On FreeBSD you can do the same using
geli [2]. In my personal opinion PostgreSQL is already complicated
enough. A few companies that hired system administrators that are too
lazy to read two or three man pages is not a reason to re-implement file
system encryption (or compression, or mirroring if that matters) in any
open source RDBMS.

[1] http://eax.me/dm-crypt/
[2] http://eax.me/freebsd-geli/

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] WIP Patch: Precalculate stable functions, infrastructure v1

[Aleksander Alekseev]

Hi Marina,

I still don't see anything particularly wrong with your patch. It
applies, passes all test, it is well test-covered and even documented.
Also I've run `make installcheck` under Valgrind and didn't find any
memory-related errors.

Is there anything that you would like to change before we call it more
or less final?

Also I would advice to add your branch to our internal buildfarm just to
make sure everything is OK on exotic platforms like Windows ;)

On Mon, May 22, 2017 at 06:32:17PM +0300, Marina Polyakova wrote:
> > Hi,
> 
> Hello!
> 
> > I've not followed this thread, but just scanned this quickly because it
> > affects execExpr* stuff.
> 
> Thank you very much for your comments! Thanks to them I have made v4 of the
> patches (as in the previous one, only planning and execution part is
> changed).
> 
> > Looks like having something like struct CachedExprState would be better,
> > than these separate allocations?  That also allows to aleviate some size
> > concerns when adding new fields (see below)
> 
> > I'd rather not have this on function scope - a) the stack pressure in
> > ExecInterpExpr is quite noticeable in profiles already b) this is going
> > to trigger warnings because of unused vars, because the compiler doesn't
> > understand that EEOP_CACHEDEXPR_IF_CACHED always follows
> > EEOP_CACHEDEXPR_SUBEXPR_END.
> > 
> > How about instead storing oldcontext in the expression itself?
> 
> Thanks, in new version I did all of it in this way.
> 
> > I'm also not sure how acceptable it is to just assume it's ok to leave
> > stuff in per_query_memory, in some cases that could prove to be
> > problematic.
> 
> I agree with you and in new version context is changed only for copying
> datum of result value (if it's a pointer, its data should be allocated in
> per_query_memory, or we will lost it for next tuples).
> 
> > Is this actually a meaningful path?  Shouldn't always have done const
> > evaluation before adding CachedExpr's?
> 
> eval_const_expressions_mutator is used several times, and one of them in
> functions for selectivity evaluation (set_baserel_size_estimates ->
> clauselist_selectivity -> clause_selectivity -> restriction_selectivity ->
> ... -> get_restriction_variable -> estimate_expression_value ->
> eval_const_expressions_mutator). In set_baserel_size_estimates function
> right after selectivity evaluation there's costs evaluation and cached
> expressions should be replaced before costs. I'm not sure that it is a good
> idea to insert cached expressions replacement in set_baserel_size_estimates,
> because in comments to it it's said "The rel's targetlist and restrictinfo
> list must have been constructed already, and rel->tuples must be set." and
> its file costsize.c is entitled as "Routines to compute (and set) relation
> sizes and path costs". So I have inserted cached expressions replacement
> just before it (but I'm not sure that I have seen all places where it should
> be inserted). What do you think about all of this?
> 
> -- 
> Marina Polyakova
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company

> From 02262b9f3a3215d3884b6ac188bafa6517ac543d Mon Sep 17 00:00:00 2001
> From: Marina Polyakova 
> Date: Mon, 15 May 2017 14:24:36 +0300
> Subject: [PATCH v4 1/3] Precalculate stable functions, infrastructure
> 
> Now in Postgresql only immutable functions are precalculated; stable functions
> are calculated for every row so in fact they don't differ from volatile
> functions.
> 
> This patch includes:
> - creation of CachedExpr node
> - usual node functions for it
> - mutator to replace nonovolatile functions' and operators' expressions by
> appropriate cached expressions.
> ---
>  src/backend/nodes/copyfuncs.c|  31 +
>  src/backend/nodes/equalfuncs.c   |  31 +
>  src/backend/nodes/nodeFuncs.c| 151 
>  src/backend/nodes/outfuncs.c |  56 
>  src/backend/nodes/readfuncs.c|  48 +++
>  src/backend/optimizer/plan/planner.c | 259 
> +++
>  src/include/nodes/nodeFuncs.h|   1 +
>  src/include/nodes/nodes.h|   1 +
>  src/include/nodes/primnodes.h|  38 +
>  9 files changed, 616 insertions(+)
> 
> diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
> index 6ad3844..f9f69a1 100644
> --- a/src/backend/nodes/copyfuncs.c
> +++ b/src/backend/nodes/copyfuncs.c
> @@ -1527,6 +1527,34 @@ _copyNullIfExpr(const NullIfExpr *from)
>   return newnode;
>  }
>  
> +static CachedExpr *
> +_copyCachedExpr(const CachedExpr *from)
> +{
> + CachedExpr *newnode = makeNode(CachedExpr);
> +
> + COPY_SCALAR_FIELD(subexprtype);
> + switch(from->subexprtype)
> + {
> + case CACHED_FUNCEXPR:
> + COPY_NODE_FIELD(subexpr.funcexpr);
> + break;
> + case CACHED_OPEXPR:
> + 

Re: [HACKERS] Logical replication & corrupted pages recovery

[Aleksander Alekseev]

Hi Konstantin,

> May be it is possible to somehow optimize it, by checking ranges of primary
> key values

It's possible. An optimization you are looking for is called Merkle
tree [1]. Particularly it's used in Riak [2].

[1] https://en.wikipedia.org/wiki/Merkle_tree
[2] http://docs.basho.com/riak/kv/2.2.3/learn/concepts/active-anti-entropy/

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Fix performance of generic atomics

[Aleksander Alekseev]

Hi Yura,

> Attached patch contains patch for all generic atomic
> functions, and also __sync_fetch_and_(or|and) for gcc, cause
> I believe GCC optimize code around intrinsic better than
> around inline assembler.
> (final performance is around 86000tps, but difference between
> 83000tps and 86000tps is not so obvious in NUMA system).

I don't see any patch in my email client or pgsql-hackers@ archive. I
would also recommend to add your patch to the nearest commitfest [1].

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

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] WIP Patch: Precalculate stable functions, infrastructure v1

[Aleksander Alekseev]

Hi Marina,

I've noticed that this patch needs a review and decided to take a look.
Here is a short summary:

* Patches apply to the master branch. There are a trailing whitespaces,
  though.
* All tests pass.
* I see 8-10% performance improvement on full text search queries.
* It seems that there is no obvious performance degradation on regular
  queries (according to pgbench).

In short, it looks very promising. 

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Error message on missing SCRAM authentication with older clients

[Aleksander Alekseev]

Hi Heikki,

> psql: SCRAM authentication requires libpq version 10 or above

Sounds good.

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Error message on missing SCRAM authentication with older clients

[Aleksander Alekseev]

Hi Magnus,

> +1, even though it's not strictly speaking a bugfix to go in a backpatch, I
> think it will help enough users that it's worth doing. And I can't see how
> it could possibly be unsafe...

Well, strictly speaking there could be applications that parse error
messages using regular expressions or something like this. But I don't
think it's something we should really bother about.

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Error message on missing SCRAM authentication with older clients

[Aleksander Alekseev]

Hi Heikki,

> psql: SCRAM authentication not supported by this version of libpq

Maybe it would be better to specify a minimum required version?

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Reversed sync check in pg_receivewal

[Aleksander Alekseev]

Hi Magnus,

> Attached patch reverses the check, and adds a failure message. I'd
> appreciate a quick review in case I have the logic backwards in my head...

Well, I can state that `make check-world` passes on my laptop and that
code seems to be right. However documentation to WalWriteMethod could
probably be improved. Currently there is none.

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] GCC 7 warnings

[Aleksander Alekseev]

Hi Peter,

> c) Expand the target buffer sizes until the warning goes away.  (Sample
> patch attached.)

I personally think it's a great patch. Unfortunately I don't have GCC
7.0 right now but at least it doesn't break anything on 6.3.1. Since
there is no rush I would suggest to add an entry to the next commitfest,
so this patch wouldn't accidentally be lost.

As a side node I believe it would be great to replace all sprintf calls
to snprintf just to be on a safe side. IIRC we did something similar to
str* procedures not a long time ago. Unless there are any objections
regarding this idea I'm willing to write such a patch.

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] [PATCH] Document the order of changing certain settings when using hot-standby servers

[Aleksander Alekseev]

Hi Yorick,

> should do so on any standby servers first, before applying the changes to

What you actually meant probably was "do so on ALL standby servers
first", right?

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] [PATCH] Remove unused argument in btree_xlog_split

[Aleksander Alekseev]

Hi Robert,

> Thanks.  Please add this to the next CommitFest, as there seems to be
> no urgency (and some risk) in committing it right before feature
> freeze.

Sure. Already done [1].

[1] https://commitfest.postgresql.org/14/1097/

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] [PATCH] Warn users about duplicate configuration parameters

[Aleksander Alekseev]

Andres, Tatsuo,

Thank you for sharing your thoughts.

> -1 - I frequently just override earlier parameters by adding an
> include at the end of the file.  Also, with postgresql.auto.conf it's
> even more common to override parameters.

> -1 from me too by the same reason Andres said.

I see no problem here. After all, it's just warnings. We can even add
a GUC that disables them specially for experienced users who knows what
she or he is doing. And/or add special case for postgresql.auto.conf.

-- 
Best regards,
Aleksander Alekseev


pgphtrO1vN7g6.pgp
Description: OpenPGP digital signature

[HACKERS] [PATCH] Warn users about duplicate configuration parameters

[Aleksander Alekseev]

Hi.

Recently I've discovered that if there are multiple values of the same
parameter in postgresql.conf PostgreSQL will silently use the last one.
It looks like not the best approach to me. For instance, user can find
the first value in the config file and expect that it will be used, etc.

I suggest to warn users about duplicated parameters. Here is a
corresponding patch.

Thoughts?

-- 
Best regards,
Aleksander Alekseev
diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l
index f01b814..6aa60a4 100644
--- a/src/backend/utils/misc/guc-file.l
+++ b/src/backend/utils/misc/guc-file.l
@@ -304,6 +304,13 @@ ProcessConfigFileInternal(GucContext context, bool applySettings, int elevel)
 			}
 			/* Now mark it as present in file */
 			record->status |= GUC_IS_IN_FILE;
+
+			/* Warn the user about duplicate configuration parameter */
+			ereport(elevel,
+(errcode(ERRCODE_DUPLICATE_OBJECT),
+errmsg("duplicate configuration parameter \"%s\" overrides previous value in file \"%s\" line %u",
+		item->name,
+		item->filename, item->sourceline)));
 		}
 		else if (strchr(item->name, GUC_QUALIFIER_SEPARATOR) == NULL)
 		{


pgp126mUyo0K4.pgp
Description: OpenPGP digital signature

Re: [HACKERS] [PATCH] Document the order of changing certain settings when using hot-standby servers

[Aleksander Alekseev]

Hi Yorick,

> Attached is an updated version of the patch that corrects the order in
> the documentation.

Looks promising. I would recommend to add this patch to the next
commitfest [1]. Otherwise there is a chance that it will be lost.

[1] https://commitfest.postgresql.org/14/

-- 
Best regards,
Aleksander Alekseev


pgpRyayBlz4tm.pgp
Description: OpenPGP digital signature

Re: [HACKERS] [PATCH] Document the order of changing certain settings when using hot-standby servers

[Aleksander Alekseev]

Hi Yorick,

> The attached patch updates the hot-standby documentation (in the high
> availability section) so it explicitly mentions that certain settings
> need to be applied to servers in a particular order. For example, it
> states that if you increase a certain setting (e.g. max_connections)
> you need to do so on a primary first, before applying it to any
> secondaries.

I'm sorry to inform you that your description of max_connection is,
lets say, not quite accurate. I've just increased max_connections on a
standby without doing anything on a master and nothing wrong happened.

-- 
Best regards,
Aleksander Alekseev


pgpqfeUI2TpqL.pgp
Description: OpenPGP digital signature

Re: [HACKERS] [PATCH] Remove unused argument in btree_xlog_split

[Aleksander Alekseev]

Hi Robert,

> Hmm.  I don't see anything wrong with that, particularly, but it seems
> we also don't need the distinction between XLOG_BTREE_SPLIT_L and
> XLOG_BTREE_SPLIT_L_ROOT or likewise between XLOG_BTREE_SPLIT_R and
> XLOG_BTREE_SPLIT_R_ROOT -- in which case I think this patch should go
> a little further and do all of that together.

Thank you for sharing your thoughts on this patch. Here is a new
version.

-- 
Best regards,
Aleksander Alekseev
diff --git a/src/backend/access/nbtree/nbtinsert.c b/src/backend/access/nbtree/nbtinsert.c
index 6dca8109fd..4de47de890 100644
--- a/src/backend/access/nbtree/nbtinsert.c
+++ b/src/backend/access/nbtree/nbtinsert.c
@@ -980,7 +980,6 @@ _bt_split(Relation rel, Buffer buf, Buffer cbuf, OffsetNumber firstright,
 rightoff;
 	OffsetNumber maxoff;
 	OffsetNumber i;
-	bool		isroot;
 	bool		isleaf;
 
 	/* Acquire a new page to split into */
@@ -1019,7 +1018,6 @@ _bt_split(Relation rel, Buffer buf, Buffer cbuf, OffsetNumber firstright,
 	lopaque = (BTPageOpaque) PageGetSpecialPointer(leftpage);
 	ropaque = (BTPageOpaque) PageGetSpecialPointer(rightpage);
 
-	isroot = P_ISROOT(oopaque);
 	isleaf = P_ISLEAF(oopaque);
 
 	/* if we're splitting this page, it won't be the root when we're done */
@@ -1330,11 +1328,7 @@ _bt_split(Relation rel, Buffer buf, Buffer cbuf, OffsetNumber firstright,
 	 (char *) rightpage + ((PageHeader) rightpage)->pd_upper,
 			((PageHeader) rightpage)->pd_special - ((PageHeader) rightpage)->pd_upper);
 
-		if (isroot)
-			xlinfo = newitemonleft ? XLOG_BTREE_SPLIT_L_ROOT : XLOG_BTREE_SPLIT_R_ROOT;
-		else
-			xlinfo = newitemonleft ? XLOG_BTREE_SPLIT_L : XLOG_BTREE_SPLIT_R;
-
+		xlinfo = newitemonleft ? XLOG_BTREE_SPLIT_L : XLOG_BTREE_SPLIT_R;
 		recptr = XLogInsert(RM_BTREE_ID, xlinfo);
 
 		PageSetLSN(origpage, recptr);
diff --git a/src/backend/access/nbtree/nbtxlog.c b/src/backend/access/nbtree/nbtxlog.c
index ac60db0d49..3610c7c7e0 100644
--- a/src/backend/access/nbtree/nbtxlog.c
+++ b/src/backend/access/nbtree/nbtxlog.c
@@ -193,7 +193,7 @@ btree_xlog_insert(bool isleaf, bool ismeta, XLogReaderState *record)
 }
 
 static void
-btree_xlog_split(bool onleft, bool isroot, XLogReaderState *record)
+btree_xlog_split(bool onleft, XLogReaderState *record)
 {
 	XLogRecPtr	lsn = record->EndRecPtr;
 	xl_btree_split *xlrec = (xl_btree_split *) XLogRecGetData(record);
@@ -996,16 +996,10 @@ btree_redo(XLogReaderState *record)
 			btree_xlog_insert(false, true, record);
 			break;
 		case XLOG_BTREE_SPLIT_L:
-			btree_xlog_split(true, false, record);
+			btree_xlog_split(true, record);
 			break;
 		case XLOG_BTREE_SPLIT_R:
-			btree_xlog_split(false, false, record);
-			break;
-		case XLOG_BTREE_SPLIT_L_ROOT:
-			btree_xlog_split(true, true, record);
-			break;
-		case XLOG_BTREE_SPLIT_R_ROOT:
-			btree_xlog_split(false, true, record);
+			btree_xlog_split(false, record);
 			break;
 		case XLOG_BTREE_VACUUM:
 			btree_xlog_vacuum(record);
diff --git a/src/backend/access/rmgrdesc/nbtdesc.c b/src/backend/access/rmgrdesc/nbtdesc.c
index fbde9d6555..00cd6e2379 100644
--- a/src/backend/access/rmgrdesc/nbtdesc.c
+++ b/src/backend/access/rmgrdesc/nbtdesc.c
@@ -35,8 +35,6 @@ btree_desc(StringInfo buf, XLogReaderState *record)
 			}
 		case XLOG_BTREE_SPLIT_L:
 		case XLOG_BTREE_SPLIT_R:
-		case XLOG_BTREE_SPLIT_L_ROOT:
-		case XLOG_BTREE_SPLIT_R_ROOT:
 			{
 xl_btree_split *xlrec = (xl_btree_split *) rec;
 
@@ -121,12 +119,6 @@ btree_identify(uint8 info)
 		case XLOG_BTREE_SPLIT_R:
 			id = "SPLIT_R";
 			break;
-		case XLOG_BTREE_SPLIT_L_ROOT:
-			id = "SPLIT_L_ROOT";
-			break;
-		case XLOG_BTREE_SPLIT_R_ROOT:
-			id = "SPLIT_R_ROOT";
-			break;
 		case XLOG_BTREE_VACUUM:
 			id = "VACUUM";
 			break;
diff --git a/src/include/access/nbtxlog.h b/src/include/access/nbtxlog.h
index d6a3085923..3cc9734b2b 100644
--- a/src/include/access/nbtxlog.h
+++ b/src/include/access/nbtxlog.h
@@ -28,8 +28,6 @@
 #define XLOG_BTREE_INSERT_META	0x20	/* same, plus update metapage */
 #define XLOG_BTREE_SPLIT_L		0x30	/* add index tuple with split */
 #define XLOG_BTREE_SPLIT_R		0x40	/* as above, new item on right */
-#define XLOG_BTREE_SPLIT_L_ROOT 0x50	/* add tuple with split of root */
-#define XLOG_BTREE_SPLIT_R_ROOT 0x60	/* as above, new item on right */
 #define XLOG_BTREE_DELETE		0x70	/* delete leaf index tuples for a page */
 #define XLOG_BTREE_UNLINK_PAGE	0x80	/* delete a half-dead page */
 #define XLOG_BTREE_UNLINK_PAGE_META 0x90		/* same, and update metapage */


signature.asc
Description: PGP signature

[HACKERS] [PATCH] Remove unused argument in btree_xlog_split

[Aleksander Alekseev]

Hi,

Turned out that there is an unused argument `isroot` in
`btree_xlog_split` procedure. Suggested patch fixes it.

This issue was discovered by Anastasia Lubennikova, coding is done by me.

-- 
Best regards,
Aleksander Alekseev
diff --git a/src/backend/access/nbtree/nbtxlog.c b/src/backend/access/nbtree/nbtxlog.c
index ac60db0d49..2db8f13cae 100644
--- a/src/backend/access/nbtree/nbtxlog.c
+++ b/src/backend/access/nbtree/nbtxlog.c
@@ -193,7 +193,7 @@ btree_xlog_insert(bool isleaf, bool ismeta, XLogReaderState *record)
 }
 
 static void
-btree_xlog_split(bool onleft, bool isroot, XLogReaderState *record)
+btree_xlog_split(bool onleft, XLogReaderState *record)
 {
 	XLogRecPtr	lsn = record->EndRecPtr;
 	xl_btree_split *xlrec = (xl_btree_split *) XLogRecGetData(record);
@@ -996,16 +996,12 @@ btree_redo(XLogReaderState *record)
 			btree_xlog_insert(false, true, record);
 			break;
 		case XLOG_BTREE_SPLIT_L:
-			btree_xlog_split(true, false, record);
-			break;
-		case XLOG_BTREE_SPLIT_R:
-			btree_xlog_split(false, false, record);
-			break;
 		case XLOG_BTREE_SPLIT_L_ROOT:
-			btree_xlog_split(true, true, record);
+			btree_xlog_split(true, record);
 			break;
+		case XLOG_BTREE_SPLIT_R:
 		case XLOG_BTREE_SPLIT_R_ROOT:
-			btree_xlog_split(false, true, record);
+			btree_xlog_split(false, record);
 			break;
 		case XLOG_BTREE_VACUUM:
 			btree_xlog_vacuum(record);


signature.asc
Description: PGP signature

Re: [HACKERS] Multiple false-positive warnings from Valgrind

[Aleksander Alekseev]

Hi Kyotaro,

> > And it seems to me that this is caused by the routines of OpenSSL.
> > When building without --with-openssl, using the fallback
> > implementations of SHA256 and RAND_bytes I see no warnings generated
> > by scram_build_verifier... I think it makes most sense to discard that
> > from the list of open items.
> 
> FWIW a document of the function says that,
> 
> https://www.openssl.org/docs/man1.0.1/crypto/RAND_bytes.html
> 
> > The contents of buf is mixed into the entropy pool before
> > retrieving the new pseudo-random bytes unless disabled at compile
> > time (see FAQ).
> 
> This isn't saying that RAND_bytes does the same thing but
> something similar can be happening there.

OK, turned out that warnings regarding uninitialized values disappear
after removing --with-openssl. That's a good thing.

What about all these memory leak reports [1]? If I see them should I just
ignore them or, if reports look false positive, suggest a patch that
modifies a Valgrind suppression file? In other words what is current
consensus in community regarding Valgrind and it's reports?

[1] http://afiskon.ru/s/47/871f1e21ef_valgrind.txt.gz

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

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

[Aleksander Alekseev]

Hi Robert,

> Has anybody done some testing of this patch with the WAL consistency
> checker?  Like, create some tables with indexes that have INCLUDE
> columns, set up a standby, enable consistency checking, pound the
> master, and see if the standby bails?

I've decided to run such a test. It looks like there is a bug indeed.

Steps to reproduce:

0. Apply a patch.
1. Build PostgreSQL using quick-build.sh [1]
2. Install master and replica using install.sh [2]
3. Download test.sql [3]
4. Run: `cat test.sql | psql`
5. In replica's logfile:

```
FATAL:  inconsistent page found, rel 1663/16384/16396, forknum 0, blkno 1
```

> Has anybody tested this patch with amcheck?  Does it break amcheck?

Amcheck doesn't complain.

[1] https://github.com/afiskon/pgscripts/blob/master/quick-build.sh
[2] https://github.com/afiskon/pgscripts/blob/master/install.sh
[3] http://afiskon.ru/s/88/93c544e6cf_test.sql

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

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

[Aleksander Alekseev]

Hi Teodor,

> I had a look on patch and played with it, seems, it looks fine. I splitted
> it to two patches: core changes (+bloom index fix) and btree itself. All
> docs are left in first patch - I'm too lazy to rewrite documentation which
> is changed in second patch.
> Any objection from reviewers to push both patches?

These patches look OK. Definitely no objections from me.

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] [COMMITTERS] pgsql: Improve performance of find_all_inheritors()

[Aleksander Alekseev]

Hi, Robert.

> I'm a little worried that this will be noticeably slower when the
> number of partitions is small, like 4 or 8 or 16.  In other places,
> we've found it necessary to support both a list-based strategy and a
> hash-based strategy to avoid regressing small cases (e.g.
> join_rel_list + join_rel_hash in PlannerInfo, ResourceArray in
> resowner.c, etc).

I've checked it [1]. Apparently in this particular case patches make code
faster on small amount of partitions as well. 

[1] https://postgr.es/m/20170324132258.GB16830%40e733.localdomain

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Re: Declarative partitioning optimization for large amount of partitions

[Aleksander Alekseev]

Hi Teodor,

Thanks a lot for a review!

> > step1 In pgstat_report_stat() you remove one by one entries from hash and
> > remove them all. Isn't it better to hash_destroy/hash_create or even let 
> > hash
> > lives in separate memory context and just resets it?

Agree, fixed.

> > step1 Again, pgstat_report_stat(), all-zero entries aren't deleted from hash
> > although they will be free from point of view of pgStatTabList.

Good point! Fixed.

-- 
Best regards,
Aleksander Alekseev
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index b704788eb5..4060f241e2 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -161,6 +161,20 @@ typedef struct TabStatusArray
 static TabStatusArray *pgStatTabList = NULL;
 
 /*
+ * pgStatTabHash entry
+ */
+typedef struct TabStatHashEntry
+{
+	Oid t_id;
+	PgStat_TableStatus* tsa_entry;
+} TabStatHashEntry;
+
+/*
+ * Hash table for O(1) t_id -> tsa_entry lookup
+ */
+static HTAB *pgStatTabHash = NULL;
+
+/*
  * Backends store per-function info that's waiting to be sent to the collector
  * in this hash table (indexed by function OID).
  */
@@ -824,6 +838,12 @@ pgstat_report_stat(bool force)
 	}
 
 	/*
+	 * pgStatTabHash is outdated on this point so we have to clean it.
+	 */
+	hash_destroy(pgStatTabHash);
+	pgStatTabHash = NULL;
+
+	/*
 	 * Send partial messages.  Make sure that any pending xact commit/abort
 	 * gets counted, even if there are no table stats to send.
 	 */
@@ -1668,59 +1688,87 @@ pgstat_initstats(Relation rel)
 }
 
 /*
+ * Make sure pgStatTabList and pgStatTabHash are initialized.
+ */
+static void
+make_sure_stat_tab_initialized()
+{
+	HASHCTL			ctl;
+	MemoryContext	new_ctx;
+
+	if(!pgStatTabList)
+	{
+		/* This is first time procedure is called */
+		pgStatTabList = (TabStatusArray *) MemoryContextAllocZero(TopMemoryContext,
+sizeof(TabStatusArray));
+	}
+
+	if(pgStatTabHash)
+		return;
+
+	/* Hash table was freed or never existed.  */
+
+	new_ctx = AllocSetContextCreate(
+		TopMemoryContext,
+		"PGStatLookupHashTableContext",
+		ALLOCSET_DEFAULT_SIZES);
+
+	memset(, 0, sizeof(ctl));
+	ctl.keysize = sizeof(Oid);
+	ctl.entrysize = sizeof(TabStatHashEntry);
+	ctl.hcxt = new_ctx;
+
+	pgStatTabHash = hash_create("pgstat t_id to tsa_entry lookup hash table",
+		TABSTAT_QUANTUM, , HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
+}
+
+/*
  * get_tabstat_entry - find or create a PgStat_TableStatus entry for rel
  */
 static PgStat_TableStatus *
 get_tabstat_entry(Oid rel_id, bool isshared)
 {
+	TabStatHashEntry* hash_entry;
 	PgStat_TableStatus *entry;
 	TabStatusArray *tsa;
-	TabStatusArray *prev_tsa;
-	int			i;
+	bool found;
+
+	make_sure_stat_tab_initialized();
 
 	/*
-	 * Search the already-used tabstat slots for this relation.
+	 * Find an entry or create a new one.
 	 */
-	prev_tsa = NULL;
-	for (tsa = pgStatTabList; tsa != NULL; prev_tsa = tsa, tsa = tsa->tsa_next)
+	hash_entry = hash_search(pgStatTabHash, _id, HASH_ENTER, );
+	if(found)
+		return hash_entry->tsa_entry;
+
+	/*
+	 * `hash_entry` was just created and now we have to fill it.
+	 * First make sure there is a free space in a last element of pgStatTabList.
+	 */
+	tsa = pgStatTabList;
+	while(tsa->tsa_used == TABSTAT_QUANTUM)
 	{
-		for (i = 0; i < tsa->tsa_used; i++)
+		if(tsa->tsa_next == NULL)
 		{
-			entry = >tsa_entries[i];
-			if (entry->t_id == rel_id)
-return entry;
+			tsa->tsa_next = (TabStatusArray *) MemoryContextAllocZero(TopMemoryContext,
+		sizeof(TabStatusArray));
 		}
 
-		if (tsa->tsa_used < TABSTAT_QUANTUM)
-		{
-			/*
-			 * It must not be present, but we found a free slot instead. Fine,
-			 * let's use this one.  We assume the entry was already zeroed,
-			 * either at creation or after last use.
-			 */
-			entry = >tsa_entries[tsa->tsa_used++];
-			entry->t_id = rel_id;
-			entry->t_shared = isshared;
-			return entry;
-		}
+		tsa = tsa->tsa_next;
 	}
 
 	/*
-	 * We ran out of tabstat slots, so allocate more.  Be sure they're zeroed.
-	 */
-	tsa = (TabStatusArray *) MemoryContextAllocZero(TopMemoryContext,
-	sizeof(TabStatusArray));
-	if (prev_tsa)
-		prev_tsa->tsa_next = tsa;
-	else
-		pgStatTabList = tsa;
-
-	/*
-	 * Use the first entry of the new TabStatusArray.
+	 * Add an entry.
 	 */
 	entry = >tsa_entries[tsa->tsa_used++];
 	entry->t_id = rel_id;
 	entry->t_shared = isshared;
+
+	/*
+	 * Add a corresponding entry to pgStatTabHash.
+	 */
+	hash_entry->tsa_entry = entry;
 	return entry;
 }
 
@@ -1732,22 +1780,19 @@ get_tabstat_entry(Oid rel_id, bool isshared)
 PgStat_TableStatus *
 find_tabstat_entry(Oid rel_id)
 {
-	PgStat_TableStatus *entry;
-	TabStatusArray *tsa;
-	int			i;
+	TabStatHashEntry* hash_entry;
 
-	for (tsa = pgStatTabList; tsa != NULL; tsa = tsa->tsa_next)
-	{
-		for (i = 0; i < tsa->tsa_used; i++)
-		{
-			entry = >ts

Re: [HACKERS] Declarative partitioning optimization for large amount of partitions

[Aleksander Alekseev]

Hi Simon,

> > I don't know which way you're thinking of fixing this, but a planner patch
> > to implement faster partition-pruning will have taken care of this, I
> > think.  As you may know, even declarative partitioned tables currently
> > depend on constraint exclusion for partition-pruning and planner's current
> > approach of handling inheritance requires to open all the child tables
> > (partitions), whereas the new approach hopefully shouldn't need to do
> > that.  I am not sure if looking for a more localized fix for this would be
> > worthwhile, although I may be wrong.
> 
> What "new approach" are we discussing?
> Is there a patch or design discussion?

I think what was meant was plans of my colleague Dmitry Ivanov to
implement partition-pruning. I've just spoke with Dmitry about this
matter. Unless there is anyone else who is already working on this
optimization we would like to work on it together. Unfortunately there
is no patch or design discussion of partition-pruning on this
commitfest.

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Re: Declarative partitioning optimization for large amount of partitions

[Aleksander Alekseev]

Also I would like to share some benchmark results.

Before applying any patches (copied from one of previous messages):

```
latency average = 1153.495 ms
latency stddev = 154.366 ms
tps = 6.061104 (including connections establishing)
tps = 6.061211 (excluding connections establishing)
```

After applying first patch (copied as well):

```
latency average = 853.862 ms
latency stddev = 112.270 ms
tps = 8.191861 (including connections establishing)
tps = 8.192028 (excluding connections establishing)
```

After applying both patches:

```
latency average = 533.646 ms
latency stddev = 86.311 ms
tps = 13.110328 (including connections establishing)
tps = 13.110608 (excluding connections establishing)
```

Small amount of partitions (2 to be exact), no patches:

```
latency average = 0.928 ms
latency stddev = 0.296 ms
tps = 7535.224897 (including connections establishing)
tps = 7536.145457 (excluding connections establishing)
```

Small amount of partitions, bot patches applied:

```
latency average = 0.915 ms
latency stddev = 0.269 ms
tps = 7638.344922 (including connections establishing)
tps = 7639.164769 (excluding connections establishing)
```

As you can see these patches don't make things worse in any regard.

Perf shows that both bottlenecks are gone. Before [1] and after [2].

[1] http://afiskon.ru/s/00/2008c4ae66_temp.png
[2] http://afiskon.ru/s/a5/fd81628a3a_temp.png

On Fri, Mar 24, 2017 at 03:17:44PM +0300, Aleksander Alekseev wrote:
> Hi Anastasia,
> 
> Thanks a lot for a review!
> 
> As was mentioned above there is also a bottleneck in find_all_inheritors
> procedure. Turned out the problem there is similar and it could be easily
> fixed as well. Corresponding patch is attached to this message. To keep
> things in order I'm attaching the previous patch as well.

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] [PATCH] Suppress Clang 3.9 warnings

[Aleksander Alekseev]

Hi Tom,

Since no one seems to be particularly excited about this patch I'm
marking it as "Returned with feedback" to save reviewers time.

On Wed, Mar 15, 2017 at 12:21:21PM -0400, Tom Lane wrote:
> Noah Misch <n...@leadboat.com> writes:
> > On Wed, Mar 15, 2017 at 10:57:15AM -0400, Tom Lane wrote:
> >> Seems like the correct solution is to
> >> absorb that fix, either by updating to a newer autoconf release or by
> >> carrying our own version of AC_CHECK_DECLS until they come out with one.
> 
> > As you mention upthread, that Autoconf commit is still newer than every
> > Autoconf release.  (Last release was 58 months ago.)  Altering configure.ac 
> > to
> > work around the bug would be reasonable, but it feels heavy relative to the
> > benefit of suppressing some warnings.
> 
> It does seem like rather a lot of work, but I think it's preferable to
> hacking up the coding in port.h.  Mainly because we could booby-trap the
> substitute AC_CHECK_DECLS to make sure we revert it whenever autoconf 2.70
> does materialize (a check on m4_PACKAGE_VERSION, like the one at
> configure.in line 22, ought to do the trick); whereas I do not think
> we'd remember to de-kluge port.h if we kluge around it there.
> 
> I'm fine with leaving it alone, too.
> 
>   regards, tom lane

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

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

[Aleksander Alekseev]

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

This patch looks good to me. As I understand we have both a complete feature 
and a consensus in a thread here. If there are no objection, I'm marking this 
patch as "Ready for Commiter".

The new status of this patch is: Ready for Committer

-- 
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] Re: Declarative partitioning optimization for large amount of partitions

[Aleksander Alekseev]

Hi Anastasia,

Thanks a lot for a review!

As was mentioned above there is also a bottleneck in find_all_inheritors
procedure. Turned out the problem there is similar and it could be easily
fixed as well. Corresponding patch is attached to this message. To keep
things in order I'm attaching the previous patch as well.

On Wed, Mar 22, 2017 at 11:53:45AM +, Anastasia Lubennikova wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:   tested, passed
> Spec compliant:   tested, passed
> Documentation:not tested
> 
> The patch looks good to me.
> It applies clearly, passes all the tests and eliminates the bottleneck 
> described in the first message.
> And as I can see from the thread, there were no objections from others,
> except a few minor comments about code style, which are fixed in the last 
> version of the patch.
> So I mark it "Ready for committer".
> 
> The new status of this patch is: Ready for Committer
> 
> -- 
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

-- 
Best regards,
Aleksander Alekseev
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 7cacb1e9b2..a22a5a37c8 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -161,6 +161,20 @@ typedef struct TabStatusArray
 static TabStatusArray *pgStatTabList = NULL;
 
 /*
+ * pgStatTabHash entry
+ */
+typedef struct TabStatHashEntry
+{
+	Oid t_id;
+	PgStat_TableStatus* tsa_entry;
+} TabStatHashEntry;
+
+/*
+ * Hash table for O(1) t_id -> tsa_entry lookup
+ */
+static HTAB *pgStatTabHash = NULL;
+
+/*
  * Backends store per-function info that's waiting to be sent to the collector
  * in this hash table (indexed by function OID).
  */
@@ -815,7 +829,13 @@ pgstat_report_stat(bool force)
 pgstat_send_tabstat(this_msg);
 this_msg->m_nentries = 0;
 			}
+
+			/*
+			 * Entry will be zeroed soon so we need to remove it from the lookup table.
+			 */
+			hash_search(pgStatTabHash, >t_id, HASH_REMOVE, NULL);
 		}
+
 		/* zero out TableStatus structs after use */
 		MemSet(tsa->tsa_entries, 0,
 			   tsa->tsa_used * sizeof(PgStat_TableStatus));
@@ -1667,59 +1687,77 @@ pgstat_initstats(Relation rel)
 }
 
 /*
+ * Make sure pgStatTabList and pgStatTabHash are initialized.
+ */
+static void
+make_sure_stat_tab_initialized()
+{
+	HASHCTL ctl;
+
+	if (pgStatTabList)
+		return;
+
+	Assert(!pgStatTabHash);
+
+	memset(, 0, sizeof(ctl));
+	ctl.keysize = sizeof(Oid);
+	ctl.entrysize = sizeof(TabStatHashEntry);
+	ctl.hcxt = TopMemoryContext;
+
+	pgStatTabHash = hash_create("pgstat t_id to tsa_entry lookup table",
+		TABSTAT_QUANTUM, , HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
+
+	pgStatTabList = (TabStatusArray *) MemoryContextAllocZero(TopMemoryContext,
+sizeof(TabStatusArray));
+}
+
+/*
  * get_tabstat_entry - find or create a PgStat_TableStatus entry for rel
  */
 static PgStat_TableStatus *
 get_tabstat_entry(Oid rel_id, bool isshared)
 {
+	TabStatHashEntry* hash_entry;
 	PgStat_TableStatus *entry;
 	TabStatusArray *tsa;
-	TabStatusArray *prev_tsa;
-	int			i;
+	bool found;
+
+	make_sure_stat_tab_initialized();
+
+	/*
+	 * Find an entry or create a new one.
+	 */
+	hash_entry = hash_search(pgStatTabHash, _id, HASH_ENTER, );
+	if(found)
+		return hash_entry->tsa_entry;
 
 	/*
-	 * Search the already-used tabstat slots for this relation.
+	 * `hash_entry` was just created and now we have to fill it.
+	 * First make sure there is a free space in a last element of pgStatTabList.
 	 */
-	prev_tsa = NULL;
-	for (tsa = pgStatTabList; tsa != NULL; prev_tsa = tsa, tsa = tsa->tsa_next)
+	tsa = pgStatTabList;
+	while(tsa->tsa_used == TABSTAT_QUANTUM)
 	{
-		for (i = 0; i < tsa->tsa_used; i++)
+		if(tsa->tsa_next == NULL)
 		{
-			entry = >tsa_entries[i];
-			if (entry->t_id == rel_id)
-return entry;
+			tsa->tsa_next = (TabStatusArray *) MemoryContextAllocZero(TopMemoryContext,
+		sizeof(TabStatusArray));
 		}
 
-		if (tsa->tsa_used < TABSTAT_QUANTUM)
-		{
-			/*
-			 * It must not be present, but we found a free slot instead. Fine,
-			 * let's use this one.  We assume the entry was already zeroed,
-			 * either at creation or after last use.
-			 */
-			entry = >tsa_entries[tsa->tsa_used++];
-			entry->t_id = rel_id;
-			entry->t_shared = isshared;
-			return entry;
-		}
+		tsa = tsa->tsa_next;
 	}
 
 	/*
-	 * We ran out of tabstat slots, so allocate more.  Be sure they're zeroed.
-	 */
-	tsa = (TabStatusArray *) MemoryContextAllocZero(TopMemoryContext,
-	sizeof(TabStatusArray));
-	if (prev_tsa)
-		prev_tsa->tsa_next = tsa;
-	else
-		pgStatTabList = tsa;
-
-	/*
-	 * Use the first entry

[HACKERS] Multiple false-positive warnings from Valgrind

[Aleksander Alekseev]

Hello.

I need a little help.

Recently I've decided to run PostgreSQL under Valgrind according to wiki
description [1]. Lots of warnings are generated [2] but it is my
understanding that all of them are false-positive. For instance I've
found these two reports particularly interesting:

```
==00:00:40:40.161 7677== Use of uninitialised value of size 8
==00:00:40:40.161 7677==at 0xA15FF5: pg_b64_encode (base64.c:68)
==00:00:40:40.161 7677==by 0x6FFE85: scram_build_verifier (auth-scram.c:348)
==00:00:40:40.161 7677==by 0x6F3F76: encrypt_password (crypt.c:171)
==00:00:40:40.161 7677==by 0x68F40C: CreateRole (user.c:403)
==00:00:40:40.161 7677==by 0x85D53A: standard_ProcessUtility (utility.c:716)
==00:00:40:40.161 7677==by 0x85CCC7: ProcessUtility (utility.c:353)
==00:00:40:40.161 7677==by 0x85BD22: PortalRunUtility (pquery.c:1165)
==00:00:40:40.161 7677==by 0x85BF20: PortalRunMulti (pquery.c:1308)
==00:00:40:40.161 7677==by 0x85B4A0: PortalRun (pquery.c:788)
==00:00:40:40.161 7677==by 0x855672: exec_simple_query (postgres.c:1101)
==00:00:40:40.161 7677==by 0x8597BB: PostgresMain (postgres.c:4066)
==00:00:40:40.161 7677==by 0x7C6322: BackendRun (postmaster.c:4317)
==00:00:40:40.161 7677==  Uninitialised value was created by a stack allocation
==00:00:40:40.161 7677==at 0x6FFDB7: scram_build_verifier (auth-scram.c:328)

==00:00:40:40.593 7677== Use of uninitialised value of size 8
==00:00:40:40.593 7677==at 0x8A7C36: hex_encode (encode.c:132)
==00:00:40:40.593 7677==by 0x6FFEF5: scram_build_verifier (auth-scram.c:355)
==00:00:40:40.593 7677==by 0x6F3F76: encrypt_password (crypt.c:171)
==00:00:40:40.593 7677==by 0x68F40C: CreateRole (user.c:403)
==00:00:40:40.593 7677==by 0x85D53A: standard_ProcessUtility (utility.c:716)
==00:00:40:40.593 7677==by 0x85CCC7: ProcessUtility (utility.c:353)
==00:00:40:40.593 7677==by 0x85BD22: PortalRunUtility (pquery.c:1165)
==00:00:40:40.593 7677==by 0x85BF20: PortalRunMulti (pquery.c:1308)
==00:00:40:40.593 7677==by 0x85B4A0: PortalRun (pquery.c:788)
==00:00:40:40.593 7677==by 0x855672: exec_simple_query (postgres.c:1101)
==00:00:40:40.593 7677==by 0x8597BB: PostgresMain (postgres.c:4066)
==00:00:40:40.593 7677==by 0x7C6322: BackendRun (postmaster.c:4317)
==00:00:40:40.593 7677==  Uninitialised value was created by a stack allocation
==00:00:40:40.593 7677==at 0x6FFDB7: scram_build_verifier (auth-scram.c:328)
==00:00:40:40.593 7677==
```

And here is what I see in GDB [3]:

```
0x00a160b4 in pg_b64_encode (src=0xffefffb10 [...] at base64.c:80
80  *p++ = _base64[(buf >> 12) & 0x3f];
(gdb) monitor get_vbits 0xffefffb10 10
  

0x008a7c36 in hex_encode (
src=0xffefffbc0 [...] at encode.c:132
132 *dst++ = hextbl[(*src >> 4) & 0xF];
(gdb) monitor get_vbits 0xffefffbc0 32
       
```

So Valgrind thinks that in both cases first argument is completely
uninitialized which is very doubtful to say the least :) There are also
lots of memory leak reports which could be found in [2].

I got a strong feeling that maybe I'm doing something wrong. Here are
exact script I'm using to build [4], install and run PostgreSQL under
Valgrind [5]. Naturally USE_VALGRIND in declared in pg_config_manual.h.
Valgrind version is 3.12 and an environment in general is Arch Linux.

Could you please give a little piece of advice? Or maybe a wiki page is
just a bit outdated?

[1] https://wiki.postgresql.org/wiki/Valgrind
[2] http://afiskon.ru/s/8a/390698e914_valgrind.tgz
[3] http://afiskon.ru/s/09/c4f6231679_pgvg.txt
[4] https://github.com/afiskon/pgscripts/blob/master/quick-build.sh
[5] https://github.com/afiskon/pgscripts/blob/master/valgrind.sh

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] [PATCH] Suppress Clang 3.9 warnings

[Aleksander Alekseev]

Hi Hoah.

Thanks a lot for a reply!

> This is wrong on platforms that do have strlcpy() in libc.

If it no too much trouble could you please explain what will happen
on such platforms? On what platform did you check it? I'm sure it
fixable. And I got a strong feeling that "wrong" could be a bit exaggerated.

> If I recall correctly, you can suppress the warnings in your own build by
> adding "ac_cv_func_strlcpy=no ac_cv_have_decl_strlcpy=no ac_cv_func_strlcat=no
> ac_cv_have_decl_strlcat=no" to the "configure" command line.

It's not exactly what I would call a solution. For instance on FreeBSD
Clang is a default compiler and many users build a software from source
code (FreeBSD ports). For some reason I doubt that many of them know
about these flags.

On Wed, Mar 15, 2017 at 02:25:38AM +, Noah Misch wrote:
> On Mon, Mar 13, 2017 at 06:35:53PM +0300, Aleksander Alekseev wrote:
> > --- a/src/include/port.h
> > +++ b/src/include/port.h
> > @@ -395,11 +395,22 @@ extern double rint(double x);
> >  extern int inet_aton(const char *cp, struct in_addr * addr);
> >  #endif
> >  
> > -#if !HAVE_DECL_STRLCAT
> > +/*
> > + * Unfortunately in case of strlcat and strlcpy we can't trust tests
> > + * executed by Autotools if Clang > 3.6 is used. Clang manages to compile
> > + * a program that shouldn't compile which causes wrong values of
> > + * HAVE_DECL_STRLCAT and HAVE_DECL_STRLCPY. More details could be found 
> > here:
> > + *
> > + * http://lists.llvm.org/pipermail/cfe-dev/2016-March/048126.html
> > + *
> > + * This is no doubt a dirty hack but apparently alternative solutions are
> > + * not much better.
> > + */
> > +#if !HAVE_DECL_STRLCAT || defined(__clang__)
> >  extern size_t strlcat(char *dst, const char *src, size_t siz);
> >  #endif
> >  
> > -#if !HAVE_DECL_STRLCPY
> > +#if !HAVE_DECL_STRLCPY || defined(__clang__)
> >  extern size_t strlcpy(char *dst, const char *src, size_t siz);
> >  #endif
> 
> This is wrong on platforms that do have strlcpy() in libc.
> 
> If I recall correctly, you can suppress the warnings in your own build by
> adding "ac_cv_func_strlcpy=no ac_cv_have_decl_strlcpy=no ac_cv_func_strlcat=no
> ac_cv_have_decl_strlcat=no" to the "configure" command line.

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] [PATCH] Suppress Clang 3.9 warnings

[Aleksander Alekseev]

Hi David,

Thank you for reminding about this patch!

Here is a new patch. I tried to make as little changes as possible. This
is no doubt not the most beautiful patch on Earth but it removes all
warnings. I anyone could suggest an approach that would be significantly
better please don't hesitate to share your ideas.

Tested on Clang 3.9.1 and GCC 6.3.1.

> Why not update the target type to "unsigned char" instead, so that no
> cast is needed and the value compatibility is checked by the compiler? I
> guess there would be some more changes (question is how much), but it
> would be cleaner.

I tried this way as well. After rebuilding PostgreSQL in 5th time I
discovered that now I have to redefine a Poiner typedef. I don't think
we can avoid using type casts. There will be just different type casts
in other places, or we'll have to break most of existing PostgreSQL
extensions.

On Mon, Mar 13, 2017 at 10:19:57AM -0400, David Steele wrote:
> Hi Aleksander,
> 
> On 2/22/17 9:43 AM, Fabien COELHO wrote:
> > 
> > Hello Aleksander,
> > 
> >> ```
> >> xloginsert.c:742:18: warning: implicit conversion from 'int' to 'char'
> >> changes value from 253 to -3 [-Wconstant-conversion]
> >> ```
> > 
> > There is a bunch of these in "xlog.c" as well, and the same code is used
> > in "pg_resetwal.c".
> > 
> >> Patch that fixes these warnings is attached to this email.
> > 
> > My 0.02€:
> > 
> > I'm not at ease at putting the thing bluntly under the carpet with a cast.
> > 
> > Why not update the target type to "unsigned char" instead, so that no
> > cast is needed and the value compatibility is checked by the compiler? I
> > guess there would be some more changes (question is how much), but it
> > would be cleaner.
> 
> There's been no discussion or new patch on this thread recently.  If you
> are planning to address the issues raised please plan to do so by
> Thursday, March 16th.
> 
> If no new patch is submitted by that date I will mark this patch
> "Returned with Feedback".
> 
> Thanks,
> -- 
> -David
> da...@pgmasters.net

-- 
Best regards,
Aleksander Alekseev
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 744360c769..6aa93b5ff7 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -5018,7 +5018,7 @@ BootStrapXLOG(void)
 	record->xl_rmid = RM_XLOG_ID;
 	recptr += SizeOfXLogRecord;
 	/* fill the XLogRecordDataHeaderShort struct */
-	*(recptr++) = XLR_BLOCK_ID_DATA_SHORT;
+	*(recptr++) = (char)XLR_BLOCK_ID_DATA_SHORT;
 	*(recptr++) = sizeof(checkPoint);
 	memcpy(recptr, , sizeof(checkPoint));
 	recptr += sizeof(checkPoint);
diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index 03b05f937f..ea8e915029 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -739,7 +739,7 @@ XLogRecordAssemble(RmgrId rmid, uint8 info,
 	if ((curinsert_flags & XLOG_INCLUDE_ORIGIN) &&
 		replorigin_session_origin != InvalidRepOriginId)
 	{
-		*(scratch++) = XLR_BLOCK_ID_ORIGIN;
+		*(scratch++) = (char)XLR_BLOCK_ID_ORIGIN;
 		memcpy(scratch, _session_origin, sizeof(replorigin_session_origin));
 		scratch += sizeof(replorigin_session_origin);
 	}
@@ -749,13 +749,13 @@ XLogRecordAssemble(RmgrId rmid, uint8 info,
 	{
 		if (mainrdata_len > 255)
 		{
-			*(scratch++) = XLR_BLOCK_ID_DATA_LONG;
+			*(scratch++) = (char)XLR_BLOCK_ID_DATA_LONG;
 			memcpy(scratch, _len, sizeof(uint32));
 			scratch += sizeof(uint32);
 		}
 		else
 		{
-			*(scratch++) = XLR_BLOCK_ID_DATA_SHORT;
+			*(scratch++) = (char)XLR_BLOCK_ID_DATA_SHORT;
 			*(scratch++) = (uint8) mainrdata_len;
 		}
 		rdt_datas_last->next = mainrdata_head;
diff --git a/src/bin/pg_resetwal/pg_resetwal.c b/src/bin/pg_resetwal/pg_resetwal.c
index 27bd9b04e7..ed6968d605 100644
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -1095,7 +1095,7 @@ WriteEmptyXLOG(void)
 	record->xl_rmid = RM_XLOG_ID;
 
 	recptr += SizeOfXLogRecord;
-	*(recptr++) = XLR_BLOCK_ID_DATA_SHORT;
+	*(recptr++) = (char)XLR_BLOCK_ID_DATA_SHORT;
 	*(recptr++) = sizeof(CheckPoint);
 	memcpy(recptr, ,
 		   sizeof(CheckPoint));
diff --git a/src/include/port.h b/src/include/port.h
index f4546016e7..0f014b72b6 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -395,11 +395,22 @@ extern double rint(double x);
 extern int	inet_aton(const char *cp, struct in_addr * addr);
 #endif
 
-#if !HAVE_DECL_STRLCAT
+/*
+ * Unfortunately in case of strlcat and strlcpy we can't trust tests
+ * executed by Autotools if Clang > 3.6 is used. Clang manages to compile
+ * a program that shouldn't compile which causes wrong values of
+ * HAVE_DECL_STRLCAT and HAVE_DECL_STRL

Re: [HACKERS] Declarative partitioning optimization for large amount of partitions

[Aleksander Alekseev]

Hi Tels,

Thanks a lot for the review!

> "corresponding"

Fixed.

> Also a question: Some one-line comments are
> 
>  /* Comment. */
> 
> while others are
> 
>  /*
>   * Comment.
>   */
> 
> Why the difference?

I'm trying to follow a code stile used in a code I'm modifying. In this
case I got an impression that second style of one-line comments is used
more often an I tried to follow it but apparently I didn't succeed :)
This is fixed as well.

On Thu, Mar 09, 2017 at 06:40:39PM -0500, Tels wrote:
> Hi Aleksander,
> 
> noticed this in your patch:
> 
> > * Add a corespinding entry to pgStatTabHash.
> 
> "corresponding"
> 
> Also a question: Some one-line comments are
> 
>  /* Comment. */
> 
> while others are
> 
>  /*
>   * Comment.
>   */
> 
> Why the difference?
> 
> Hope this helps,
> 
> Tels

-- 
Best regards,
Aleksander Alekseev
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 7cacb1e9b2..a22a5a37c8 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -161,6 +161,20 @@ typedef struct TabStatusArray
 static TabStatusArray *pgStatTabList = NULL;
 
 /*
+ * pgStatTabHash entry
+ */
+typedef struct TabStatHashEntry
+{
+	Oid t_id;
+	PgStat_TableStatus* tsa_entry;
+} TabStatHashEntry;
+
+/*
+ * Hash table for O(1) t_id -> tsa_entry lookup
+ */
+static HTAB *pgStatTabHash = NULL;
+
+/*
  * Backends store per-function info that's waiting to be sent to the collector
  * in this hash table (indexed by function OID).
  */
@@ -815,7 +829,13 @@ pgstat_report_stat(bool force)
 pgstat_send_tabstat(this_msg);
 this_msg->m_nentries = 0;
 			}
+
+			/*
+			 * Entry will be zeroed soon so we need to remove it from the lookup table.
+			 */
+			hash_search(pgStatTabHash, >t_id, HASH_REMOVE, NULL);
 		}
+
 		/* zero out TableStatus structs after use */
 		MemSet(tsa->tsa_entries, 0,
 			   tsa->tsa_used * sizeof(PgStat_TableStatus));
@@ -1667,59 +1687,77 @@ pgstat_initstats(Relation rel)
 }
 
 /*
+ * Make sure pgStatTabList and pgStatTabHash are initialized.
+ */
+static void
+make_sure_stat_tab_initialized()
+{
+	HASHCTL ctl;
+
+	if (pgStatTabList)
+		return;
+
+	Assert(!pgStatTabHash);
+
+	memset(, 0, sizeof(ctl));
+	ctl.keysize = sizeof(Oid);
+	ctl.entrysize = sizeof(TabStatHashEntry);
+	ctl.hcxt = TopMemoryContext;
+
+	pgStatTabHash = hash_create("pgstat t_id to tsa_entry lookup table",
+		TABSTAT_QUANTUM, , HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
+
+	pgStatTabList = (TabStatusArray *) MemoryContextAllocZero(TopMemoryContext,
+sizeof(TabStatusArray));
+}
+
+/*
  * get_tabstat_entry - find or create a PgStat_TableStatus entry for rel
  */
 static PgStat_TableStatus *
 get_tabstat_entry(Oid rel_id, bool isshared)
 {
+	TabStatHashEntry* hash_entry;
 	PgStat_TableStatus *entry;
 	TabStatusArray *tsa;
-	TabStatusArray *prev_tsa;
-	int			i;
+	bool found;
+
+	make_sure_stat_tab_initialized();
+
+	/*
+	 * Find an entry or create a new one.
+	 */
+	hash_entry = hash_search(pgStatTabHash, _id, HASH_ENTER, );
+	if(found)
+		return hash_entry->tsa_entry;
 
 	/*
-	 * Search the already-used tabstat slots for this relation.
+	 * `hash_entry` was just created and now we have to fill it.
+	 * First make sure there is a free space in a last element of pgStatTabList.
 	 */
-	prev_tsa = NULL;
-	for (tsa = pgStatTabList; tsa != NULL; prev_tsa = tsa, tsa = tsa->tsa_next)
+	tsa = pgStatTabList;
+	while(tsa->tsa_used == TABSTAT_QUANTUM)
 	{
-		for (i = 0; i < tsa->tsa_used; i++)
+		if(tsa->tsa_next == NULL)
 		{
-			entry = >tsa_entries[i];
-			if (entry->t_id == rel_id)
-return entry;
+			tsa->tsa_next = (TabStatusArray *) MemoryContextAllocZero(TopMemoryContext,
+		sizeof(TabStatusArray));
 		}
 
-		if (tsa->tsa_used < TABSTAT_QUANTUM)
-		{
-			/*
-			 * It must not be present, but we found a free slot instead. Fine,
-			 * let's use this one.  We assume the entry was already zeroed,
-			 * either at creation or after last use.
-			 */
-			entry = >tsa_entries[tsa->tsa_used++];
-			entry->t_id = rel_id;
-			entry->t_shared = isshared;
-			return entry;
-		}
+		tsa = tsa->tsa_next;
 	}
 
 	/*
-	 * We ran out of tabstat slots, so allocate more.  Be sure they're zeroed.
-	 */
-	tsa = (TabStatusArray *) MemoryContextAllocZero(TopMemoryContext,
-	sizeof(TabStatusArray));
-	if (prev_tsa)
-		prev_tsa->tsa_next = tsa;
-	else
-		pgStatTabList = tsa;
-
-	/*
-	 * Use the first entry of the new TabStatusArray.
+	 * Add an entry.
 	 */
 	entry = >tsa_entries[tsa->tsa_used++];
 	entry->t_id = rel_id;
 	entry->t_shared = isshared;
+
+	/*
+	 * Add a corresponding entry to pgStatTabHash.
+	 */
+	hash_entry->tsa_entry = entry;
 	return entry;
 }
 
@@ -1731,22 +1769,19 @@ get_tabstat_entry(Oid rel_id, bool issha

Re: [HACKERS] Declarative partitioning optimization for large amount of partitions

[Aleksander Alekseev]

Hi Amit,

> Sorry, I didn't mean to dissuade you from trying those
> micro-optimizations.  If general inheritance cases can benefit from it
> (which, until we have a different method, will be used by partitioned
> tables as well), I think we should try it.

OK, I'll see what could be done here as well then.

On Tue, Mar 07, 2017 at 10:55:12AM +0900, Amit Langote wrote:
> Hi Aleksander,
> 
> On 2017/03/07 0:22, Aleksander Alekseev wrote:
> > Hello.
> > 
> > OK, here is a patch.
> > 
> > Benchmark, before:
> > 
> > ```
> > number of transactions actually processed: 1823
> > latency average = 1153.495 ms
> > latency stddev = 154.366 ms
> > tps = 6.061104 (including connections establishing)
> > tps = 6.061211 (excluding connections establishing)
> > ```
> > 
> > Benchmark, after:
> > 
> > ```
> > number of transactions actually processed: 2462
> > latency average = 853.862 ms
> > latency stddev = 112.270 ms
> > tps = 8.191861 (including connections establishing)
> > tps = 8.192028 (excluding connections establishing)
> > ```
> > 
> > +35% TPS, just as expected. Feel free to run your own benchmarks on
> > different datasets and workloads. `perf top` shows that first bottleneck
> > is completely eliminated.
> 
> That seems like a good gain.
> 
> > I did nothing about the second bottleneck
> > since as Amit mentioned partition-pruning should solve this anyway and
> > apparently any micro-optimizations don't worth an effort.
> 
> Sorry, I didn't mean to dissuade you from trying those
> micro-optimizations.  If general inheritance cases can benefit from it
> (which, until we have a different method, will be used by partitioned
> tables as well), I think we should try it.
> 
> Thanks,
> Amit
> 
> 

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Declarative partitioning optimization for large amount of partitions

[Aleksander Alekseev]

Hi, Andres

Thanks a lot for the review!

> Why are we keeping that list / the "batch" allocation pattern? It
> doesn't actually seem useful to me after these changes.  Given that we
> don't shrink hash-tables, we could just use the hash-table memory for
> this, no?

I don't think we can do that. According to comments:

```
 * NOTE: once allocated, TabStatusArray structures are never moved or deleted
 [...]
 * This allows relcache pgstat_info pointers to be treated as long-lived data,
 * avoiding repeated searches in pgstat_initstats() when a relation is
 * repeatedly opened during a transaction.
```

It is my understanding that dynahash can't give same guarantees.

> 'faster ... lookup' doesn't strike me as a useful comment, because it's
> only useful in relation of the current code to the new code.

Agree, fixed to 'O(1) ... lookup'.

> It's not really freed...

Right, it's actually zeroed. Fixed.

> Arguably it'd be better to HASH_ENTER directly here, instead of doing
> two lookups.

Good point. Fixed.

> Think it'd be better to move the hash creation into its own function.

Agree, fixed.

On Mon, Mar 06, 2017 at 12:01:50PM -0800, Andres Freund wrote:
> Hi,
> 
> This issue has bothered me in non-partitioned use-cases recently, so
> thanks for taking it up.
> 
> 
> On 2017-03-06 18:22:17 +0300, Aleksander Alekseev wrote:
> > diff --git a/src/backend/postmaster/pgstat.c 
> > b/src/backend/postmaster/pgstat.c
> > index 2fb9a8bf58..fa906e7930 100644
> > --- a/src/backend/postmaster/pgstat.c
> > +++ b/src/backend/postmaster/pgstat.c
> > @@ -160,6 +160,16 @@ typedef struct TabStatusArray
> >  
> >  static TabStatusArray *pgStatTabList = NULL;
> 
> Why are we keeping that list / the "batch" allocation pattern? It
> doesn't actually seem useful to me after these changes.  Given that we
> don't shrink hash-tables, we could just use the hash-table memory for
> this, no?
> 
> I think a separate list that only contains things that are "active" in
> the current transaction makes sense, because the current logic requires
> iterating over a potentially very large array at transaction commit.
> 
> 
> > +/* pgStatTabHash entry */
> > +typedef struct TabStatHashEntry
> > +{
> > +   Oid t_id;
> > +   PgStat_TableStatus* tsa_entry;
> > +} TabStatHashEntry;
> > +
> > +/* Hash table for faster t_id -> tsa_entry lookup */
> > +static HTAB *pgStatTabHash = NULL;
> > +
> 
> 'faster ... lookup' doesn't strike me as a useful comment, because it's
> only useful in relation of the current code to the new code.
> 
> 
> 
> >  /*
> >   * Backends store per-function info that's waiting to be sent to the 
> > collector
> >   * in this hash table (indexed by function OID).
> > @@ -815,7 +825,13 @@ pgstat_report_stat(bool force)
> > pgstat_send_tabstat(this_msg);
> > this_msg->m_nentries = 0;
> > }
> > +
> > +   /*
> > +* Entry will be freed soon so we need to remove it 
> > from the lookup table.
> > +*/
> > +   hash_search(pgStatTabHash, >t_id, HASH_REMOVE, 
> > NULL);
> > }
> 
> It's not really freed...
> 
> 
> >  static PgStat_TableStatus *
> >  get_tabstat_entry(Oid rel_id, bool isshared)
> >  {
> > +   TabStatHashEntry* hash_entry;
> > PgStat_TableStatus *entry;
> > TabStatusArray *tsa;
> > -   TabStatusArray *prev_tsa;
> > -   int i;
> > +
> > +   /* Try to find an entry */
> > +   entry = find_tabstat_entry(rel_id);
> > +   if(entry != NULL)
> > +   return entry;
> 
> Arguably it'd be better to HASH_ENTER directly here, instead of doing
> two lookups.
> 
> 
> > /*
> > -* Search the already-used tabstat slots for this relation.
> > +* Entry doesn't exist - creating one.
> > +* First make sure there is a free space in a last element of 
> > pgStatTabList.
> >  */
> > -   prev_tsa = NULL;
> > -   for (tsa = pgStatTabList; tsa != NULL; prev_tsa = tsa, tsa = 
> > tsa->tsa_next)
> > +   if (!pgStatTabList)
> > {
> > -   for (i = 0; i < tsa->tsa_used; i++)
> > -   {
> > -   entry = >tsa_entries[i];
> > -   if (entry->t_id == rel_id)
> > -   return entry;
> > -   }
> > +   HASHCTL ctl;
> >  
> > -   if (tsa->tsa_used < TABSTAT_QUANTUM)
> >

Re: [HACKERS] Declarative partitioning optimization for large amount of partitions

[Aleksander Alekseev]

Hello.

OK, here is a patch.

Benchmark, before:

```
number of transactions actually processed: 1823
latency average = 1153.495 ms
latency stddev = 154.366 ms
tps = 6.061104 (including connections establishing)
tps = 6.061211 (excluding connections establishing)
```

Benchmark, after:

```
number of transactions actually processed: 2462
latency average = 853.862 ms
latency stddev = 112.270 ms
tps = 8.191861 (including connections establishing)
tps = 8.192028 (excluding connections establishing)
```

+35% TPS, just as expected. Feel free to run your own benchmarks on
different datasets and workloads. `perf top` shows that first bottleneck
is completely eliminated. I did nothing about the second bottleneck
since as Amit mentioned partition-pruning should solve this anyway and
apparently any micro-optimizations don't worth an effort.

Also I checked test coverage using lcov to make sure that this code is
test covered. An exact script I'm using could be found here [1].

[1] https://github.com/afiskon/pgscripts/blob/master/code-coverage.sh

On Wed, Mar 01, 2017 at 10:36:24AM +0900, Amit Langote wrote:
> Hi,
> 
> On 2017/02/28 23:25, Aleksander Alekseev wrote:
> > Hello.
> > 
> > I decided to figure out whether current implementation of declarative
> > partitioning has any bottlenecks when there is a lot of partitions. Here
> > is what I did [1].
> 
> Thanks for sharing.
> 
> > Then:
> > 
> > ```
> > # 2580 is some pk that exists
> > echo 'select * from part_test where pk = 2580;' > t.sql
> > pgbench -j 7 -c 7 -f t.sql -P 1 -T 300 eax
> > ```
> > 
> > `perf top` showed to bottlenecks [2]. A stacktrace for the first one
> > looks like this [3]:
> > 
> > ```
> > 0x007a42e2 in get_tabstat_entry (rel_id=25696, isshared=0 '\000') 
> > at pgstat.c:1689
> > 1689if (entry->t_id == rel_id)
> > #0  0x007a42e2 in get_tabstat_entry (rel_id=25696, isshared=0 
> > '\000') at pgstat.c:1689
> > #1  0x007a4275 in pgstat_initstats (rel=0x7f4af3fd41f8) at 
> > pgstat.c:1666
> > #2  0x004c7090 in relation_open (relationId=25696, lockmode=0) at 
> > heapam.c:1137
> > #3  0x004c72c9 in heap_open (relationId=25696, lockmode=0) at 
> > heapam.c:1291
> > (skipped)
> > ```
> > 
> > And here is a stacktrace for the second bottleneck [4]:
> > 
> > ```
> > 0x00584fb1 in find_all_inheritors (parentrelId=16393, lockmode=1, 
> > numparents=0x0) at pg_inherits.c:199
> > 199 forboth(lo, rels_list, li, rel_numparents)
> > #0  0x00584fb1 in find_all_inheritors (parentrelId=16393, 
> > lockmode=1, numparents=0x0) at pg_inherits.c:199
> > #1  0x0077fc9f in expand_inherited_rtentry (root=0x1badcb8, 
> > rte=0x1b630b8, rti=1) at prepunion.c:1408
> > #2  0x0077fb67 in expand_inherited_tables (root=0x1badcb8) at 
> > prepunion.c:1335
> > #3  0x00767526 in subquery_planner (glob=0x1b63cc0, 
> > parse=0x1b62fa0, parent_root=0x0, hasRecursion=0 '\000', tuple_fraction=0) 
> > at planner.c:568
> > (skipped)
> > ```
> > 
> > The first one could be easily fixed by introducing a hash table
> > (rel_id -> pgStatList entry). Perhaps hash table should be used only
> > after some threshold. Unless there are any objections I will send a
> > corresponding patch shortly.
> 
> I have never thought about this one really.
> 
> > I didn't explored the second bottleneck closely yet but at first glance
> > it doesn't look much more complicated.
> 
> I don't know which way you're thinking of fixing this, but a planner patch
> to implement faster partition-pruning will have taken care of this, I
> think.  As you may know, even declarative partitioned tables currently
> depend on constraint exclusion for partition-pruning and planner's current
> approach of handling inheritance requires to open all the child tables
> (partitions), whereas the new approach hopefully shouldn't need to do
> that.  I am not sure if looking for a more localized fix for this would be
> worthwhile, although I may be wrong.
> 
> Thanks,
> Amit
> 
> 

-- 
Best regards,
Aleksander Alekseev
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 2fb9a8bf58..fa906e7930 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -160,6 +160,16 @@ typedef struct TabStatusArray
 
 static TabStatusArray *pgStatTabList = NULL;
 
+/* pgStatTabHash entry */
+typedef struct TabStatHashEntry
+{
+	Oid t_id;
+	PgStat_TableStatus* tsa_entry;
+} TabStatHashEntry;
+
+/* Hash table for faster t_id -> tsa_entry lookup 

Re: [HACKERS] [POC] hash partitioning

[Aleksander Alekseev]

> We can achieve desired result through creating a separate partitioned table
> and making the DETACH/ATTACH manipulation, though. But IMO it's not flexible
> case.

I think it would be great to allow end user to decide. If user is
not interested in subpartitions he or she can use syntax like 'CREATE
TABLE ... PARTITION BY HAHS(i) PARTITIONS 3 CREATE AUTOMATICALLY;' or
maybe a build-in procedure for this. Otherwise there is also
ATTACH/DETACH syntax available.

Anyway all of this is something that could be discussed infinitely and
not necessarily should be included in this concrete patch. We could
probably agree that 3 or 4 separately discussed, reviewed and tested
patches are better than one huge patch that will be moved to the next
commitfest because of disagreements regarding a syntax.

On Wed, Mar 01, 2017 at 05:10:34PM +0300, Maksim Milyutin wrote:
> On 01.03.2017 05:14, Amit Langote wrote:
> > Nagata-san,
> > 
> > > A partition table can be create as bellow;
> > > 
> > >  CREATE TABLE h1 PARTITION OF h;
> > >  CREATE TABLE h2 PARTITION OF h;
> > >  CREATE TABLE h3 PARTITION OF h;
> > > 
> > > FOR VALUES clause cannot be used, and the partition bound is
> > > calclulated automatically as partition index of single integer value.
> > > 
> > > When trying create partitions more than the number specified
> > > by PARTITIONS, it gets an error.
> > > 
> > > postgres=# create table h4 partition of h;
> > > ERROR:  cannot create hash partition more than 3 for h
> > 
> > Instead of having to create each partition individually, wouldn't it be
> > better if the following command
> > 
> > CREATE TABLE h (i int) PARTITION BY HASH (i) PARTITIONS 3;
> > 
> > created the partitions *automatically*?
> 
> It's a good idea but in this case we can't create hash-partition that is
> also partitioned table, and as a consequence we are unable to create
> subpartitions. My understanding is that the table can be partitioned only
> using CREATE TABLE statement, not ALTER TABLE. For this reason the new
> created partitions are only regular tables.
> 
> We can achieve desired result through creating a separate partitioned table
> and making the DETACH/ATTACH manipulation, though. But IMO it's not flexible
> case.
> 
> It would be a good thing if a regular table could be partitioned through
> separate command. Then your idea would not be restrictive.
> 
> 
> -- 
> Maksim Milyutin
> 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

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] [POC] hash partitioning

[Aleksander Alekseev]

Hi, Yugo.

Today I've had an opportunity to take a closer look on this patch. Here are
a few things that bother me.

1a) There are missing commends here:

```
--- a/src/include/catalog/pg_partitioned_table.h
+++ b/src/include/catalog/pg_partitioned_table.h
@@ -33,6 +33,9 @@ CATALOG(pg_partitioned_table,3350) BKI_WITHOUT_OIDS
charpartstrat;  /* partitioning strategy */
int16   partnatts;  /* number of partition key columns */

+   int16   partnparts;
+   Oid parthashfunc;
+
```

1b) ... and here:

```
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -730,11 +730,14 @@ typedef struct PartitionSpec
NodeTag type;
char   *strategy;   /* partitioning strategy ('list' or 'range') */
List   *partParams; /* List of PartitionElems */
+   int partnparts;
+   List   *hashfunc;
int location;   /* token location, or -1 if unknown */
 } PartitionSpec;
```

2) I believe new empty lines in patches are generally not welcomed by
community:

```
@@ -49,6 +52,8 @@ CATALOG(pg_partitioned_table,3350) BKI_WITHOUT_OIDS
pg_node_tree partexprs; /* list of expressions in the partition key;
 * one item for each zero entry in partattrs[] 
*/
 #endif
+
+
 } FormData_pg_partitioned_table;
```

3) One test fails on my laptop (Arch Linux, x64) [1]:

```
***
*** 344,350 
  CREATE TABLE partitioned (
a int
  ) PARTITION BY HASH (a);
! ERROR:  unrecognized partitioning strategy "hash"
  -- specified column must be present in the table
  CREATE TABLE partitioned (
a int
--- 344,350 
  CREATE TABLE partitioned (
a int
  ) PARTITION BY HASH (a);
! ERROR:  number of partitions must be specified for hash partition
  -- specified column must be present in the table
  CREATE TABLE partitioned (
a int
```

Exact script I'm using for building and testing PostgreSQL could be
found here [2].

4) As I already mentioned - missing documentation.

In general patch looks quite good to me. I personally believe it has all
the changes to be accepted in current commitfest. Naturally if community
will come to a consensus regarding keywords, whether all partitions
should be created automatically, etc :)

[1] http://afiskon.ru/s/dd/20cbe21934_regression.diffs.txt
[2] http://afiskon.ru/s/76/a4fb71739c_full-build.sh.txt

On Wed, Mar 01, 2017 at 06:10:10PM +0900, Yugo Nagata wrote:
> Hi Aleksander,
> 
> On Tue, 28 Feb 2017 18:05:36 +0300
> Aleksander Alekseev <a.aleks...@postgrespro.ru> wrote:
> 
> > Hi, Yugo.
> > 
> > Looks like a great feature! I'm going to take a closer look on your code
> > and write a feedback shortly. For now I can only tell that you forgot
> > to include some documentation in the patch.
> 
> Thank you for looking into it. I'm forward to your feedback.
> This is a proof of concept patch and additional documentation
> is not included. I'll add this after reaching a consensus
> on the specification of the feature.
> 
> > 
> > I've added a corresponding entry to current commitfest [1]. Hope you
> > don't mind. If it's not too much trouble could you please register on a
> > commitfest site and add yourself to this entry as an author? I'm pretty
> > sure someone is using this information for writing release notes or
> > something like this.
> 
> Thank you for registering it to the commitfest. I have added me as an auther.
> 
> > 
> > [1] https://commitfest.postgresql.org/13/1059/
> > 
> > On Tue, Feb 28, 2017 at 11:33:13PM +0900, Yugo Nagata wrote:
> > > Hi all,
> > > 
> > > Now we have a declarative partitioning, but hash partitioning is not
> > > implemented yet. Attached is a POC patch to add the hash partitioning
> > > feature. I know we will need more discussions about the syntax and other
> > > specifications before going ahead the project, but I think this runnable
> > > code might help to discuss what and how we implement this.
> > > 
> > > * Description
> > > 
> > > In this patch, the hash partitioning implementation is basically based
> > > on the list partitioning mechanism. However, partition bounds cannot be
> > > specified explicitly, but this is used internally as hash partition
> > > index, which is calculated when a partition is created or attached.
> > > 
> > > The tentative syntax to create a partitioned table is as bellow;
> > > 
> > >  CREATE TABLE h (i int) PARTITION BY HASH(i) PARTITIONS 3 USING hashint4;
> > > 
> > > The number of partitions is specified by PARTITIONS, which is currently
> > > constant and cannot be changed, but I think this is needed to be changed 
> > &g

Re: [HACKERS] [POC] hash partitioning

[Aleksander Alekseev]

> @@ -33,6 +33,9 @@ CATALOG(pg_partitioned_table,3350) BKI_WITHOUT_OIDS
>   charpartstrat;  /* partitioning strategy */
>   int16   partnatts;  /* number of partition key 
> columns */
>  
> + int16   partnparts;
> + Oid parthashfunc;
> +
>   /*
>* variable-length fields start here, but we allow direct access to
>* partattrs via the C struct.  That's because the first variable-length
> @@ -49,6 +52,8 @@ CATALOG(pg_partitioned_table,3350) BKI_WITHOUT_OIDS
>   pg_node_tree partexprs; /* list of expressions in the partition 
> key;
>* one item for 
> each zero entry in partattrs[] */
>  #endif
> +
> +
>  } FormData_pg_partitioned_table;
>  
>  /* 
> @@ -62,13 +67,15 @@ typedef FormData_pg_partitioned_table 
> *Form_pg_partitioned_table;
>   *   compiler constants for pg_partitioned_table
>   * 
>   */
> -#define Natts_pg_partitioned_table   7
> +#define Natts_pg_partitioned_table   9
>  #define Anum_pg_partitioned_table_partrelid  1
>  #define Anum_pg_partitioned_table_partstrat  2
>  #define Anum_pg_partitioned_table_partnatts  3
> -#define Anum_pg_partitioned_table_partattrs  4
> -#define Anum_pg_partitioned_table_partclass  5
> -#define Anum_pg_partitioned_table_partcollation 6
> -#define Anum_pg_partitioned_table_partexprs  7
> +#define Anum_pg_partitioned_table_partnparts 4
> +#define Anum_pg_partitioned_table_parthashfunc   5
> +#define Anum_pg_partitioned_table_partattrs  6
> +#define Anum_pg_partitioned_table_partclass  7
> +#define Anum_pg_partitioned_table_partcollation 8
> +#define Anum_pg_partitioned_table_partexprs  9
>  
>  #endif   /* PG_PARTITIONED_TABLE_H */
> diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
> index 5afc3eb..1c3474f 100644
> --- a/src/include/nodes/parsenodes.h
> +++ b/src/include/nodes/parsenodes.h
> @@ -730,11 +730,14 @@ typedef struct PartitionSpec
>   NodeTag type;
>   char   *strategy;   /* partitioning strategy ('list' or 
> 'range') */
>   List   *partParams; /* List of PartitionElems */
> + int partnparts;
> + List   *hashfunc;
>   int location;   /* token location, or 
> -1 if unknown */
>  } PartitionSpec;
>  
>  #define PARTITION_STRATEGY_LIST  'l'
>  #define PARTITION_STRATEGY_RANGE 'r'
> +#define PARTITION_STRATEGY_HASH  'h'
>  
>  /*
>   * PartitionBoundSpec - a partition bound specification
> diff --git a/src/include/parser/kwlist.h b/src/include/parser/kwlist.h
> index 985d650..0597939 100644
> --- a/src/include/parser/kwlist.h
> +++ b/src/include/parser/kwlist.h
> @@ -180,6 +180,7 @@ PG_KEYWORD("greatest", GREATEST, COL_NAME_KEYWORD)
>  PG_KEYWORD("group", GROUP_P, RESERVED_KEYWORD)
>  PG_KEYWORD("grouping", GROUPING, COL_NAME_KEYWORD)
>  PG_KEYWORD("handler", HANDLER, UNRESERVED_KEYWORD)
> +PG_KEYWORD("hash", HASH, UNRESERVED_KEYWORD)
>  PG_KEYWORD("having", HAVING, RESERVED_KEYWORD)
>  PG_KEYWORD("header", HEADER_P, UNRESERVED_KEYWORD)
>  PG_KEYWORD("hold", HOLD, UNRESERVED_KEYWORD)
> @@ -291,6 +292,7 @@ PG_KEYWORD("parallel", PARALLEL, UNRESERVED_KEYWORD)
>  PG_KEYWORD("parser", PARSER, UNRESERVED_KEYWORD)
>  PG_KEYWORD("partial", PARTIAL, UNRESERVED_KEYWORD)
>  PG_KEYWORD("partition", PARTITION, UNRESERVED_KEYWORD)
> +PG_KEYWORD("partitions", PARTITIONS, UNRESERVED_KEYWORD)
>  PG_KEYWORD("passing", PASSING, UNRESERVED_KEYWORD)
>  PG_KEYWORD("password", PASSWORD, UNRESERVED_KEYWORD)
>  PG_KEYWORD("placing", PLACING, RESERVED_KEYWORD)
> diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
> index a617a7c..660adfb 100644
> --- a/src/include/utils/rel.h
> +++ b/src/include/utils/rel.h
> @@ -62,6 +62,9 @@ typedef struct PartitionKeyData
>   Oid*partopcintype;  /* OIDs of opclass declared 
> input data types */
>   FmgrInfo   *partsupfunc;/* lookup info for support funcs */
>  
> + int16   partnparts; /* number of hash partitions */
> + Oid parthashfunc;   /* OID of hash function */
> +
>   /* Partitioning collation per attribute */
>   Oid*partcollation;
>  

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


-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

[HACKERS] Declarative partitioning optimization for large amount of partitions

[Aleksander Alekseev]

Hello.

I decided to figure out whether current implementation of declarative
partitioning has any bottlenecks when there is a lot of partitions. Here
is what I did [1].

```
-- init schema

\timing on

CREATE TABLE part_test (pk int not null, k int, v varchar(128)) PARTITION BY 
RANGE(pk);

do $$
declare
i integer;
begin
for i in 1 .. 1
loop
raise notice 'i = %', i;
execute ('CREATE TABLE part_test_' || i ||
 ' PARTITION OF part_test FOR VALUES FROM (' ||
 (1 + (i-1)*1000) || ') to (' || ( (i * 1000) + 1) || ');'
);
end loop;
end $$;

-- fill tables with some data

do $$
declare
i integer;
begin
for i in 1 .. 100*1000
loop
raise notice 'i = %', i;
execute ('insert into part_test values ( ceil(random()*(1-1)*1000), 
ceil(random()*1*1000),  || ceil(random()*1*1000) );');
end loop;
end $$;
```

Then:

```
# 2580 is some pk that exists
echo 'select * from part_test where pk = 2580;' > t.sql
pgbench -j 7 -c 7 -f t.sql -P 1 -T 300 eax
```

`perf top` showed to bottlenecks [2]. A stacktrace for the first one
looks like this [3]:

```
0x007a42e2 in get_tabstat_entry (rel_id=25696, isshared=0 '\000') at 
pgstat.c:1689
1689if (entry->t_id == rel_id)
#0  0x007a42e2 in get_tabstat_entry (rel_id=25696, isshared=0 '\000') 
at pgstat.c:1689
#1  0x007a4275 in pgstat_initstats (rel=0x7f4af3fd41f8) at pgstat.c:1666
#2  0x004c7090 in relation_open (relationId=25696, lockmode=0) at 
heapam.c:1137
#3  0x004c72c9 in heap_open (relationId=25696, lockmode=0) at 
heapam.c:1291
(skipped)
```

And here is a stacktrace for the second bottleneck [4]:

```
0x00584fb1 in find_all_inheritors (parentrelId=16393, lockmode=1, 
numparents=0x0) at pg_inherits.c:199
199 forboth(lo, rels_list, li, rel_numparents)
#0  0x00584fb1 in find_all_inheritors (parentrelId=16393, lockmode=1, 
numparents=0x0) at pg_inherits.c:199
#1  0x0077fc9f in expand_inherited_rtentry (root=0x1badcb8, 
rte=0x1b630b8, rti=1) at prepunion.c:1408
#2  0x0077fb67 in expand_inherited_tables (root=0x1badcb8) at 
prepunion.c:1335
#3  0x00767526 in subquery_planner (glob=0x1b63cc0, parse=0x1b62fa0, 
parent_root=0x0, hasRecursion=0 '\000', tuple_fraction=0) at planner.c:568
(skipped)
```

The first one could be easily fixed by introducing a hash table
(rel_id -> pgStatList entry). Perhaps hash table should be used only
after some threshold. Unless there are any objections I will send a
corresponding patch shortly.

I didn't explored the second bottleneck closely yet but at first glance
it doesn't look much more complicated.

Please don't hesitate to share your thoughts regarding this matter.

[1] http://afiskon.ru/s/e3/5f47af9102_benchmark.txt
[2] http://afiskon.ru/s/00/2008c4ae66_temp.png
[3] http://afiskon.ru/s/23/650f0afc89_stack.txt
[4] http://afiskon.ru/s/03/a7e685a4db_stack2.txt

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] [PATCH] Suppress Clang 3.9 warnings

[Aleksander Alekseev]

In theory - could we just always use our internal strl* implementations? 

On Mon, Feb 20, 2017 at 09:26:44AM -0500, Tom Lane wrote:
> Aleksander Alekseev <a.aleks...@postgrespro.ru> writes:
> > I've just tried to build PostgreSQL with Clang 3.9.1 (default version
> > currently available in Arch Linux) and noticed that it outputs lots of
> > warning messages. Most of them are result of a bug in Clang itself:
> > 
> > postinit.c:846:3: note: include the header  or explicitly
> > provide a declaration for 'strlcpy'
> 
> It might be an incompatibility with the platform-supplied string.h
> rather than an outright bug, but yeah, that's pretty annoying.
> 
> > The rest of warnings looks more like something we could easily deal with:
> 
> It's hard to get excited about these if there are going to be hundreds
> of the other ones ...
> 
>   regards, tom lane

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

[HACKERS] [PATCH] Suppress Clang 3.9 warnings

[Aleksander Alekseev]

Hello.

I've just tried to build PostgreSQL with Clang 3.9.1 (default version
currently available in Arch Linux) and noticed that it outputs lots of
warning messages. Most of them are result of a bug in Clang itself:

```
postinit.c:846:3: note: include the header  or explicitly
provide a declaration for 'strlcpy'
```

I've reported it to Clang developers almost a year ago but apparently
no one cares. You can find all the details in a corresponding thread
[1]. Frankly I'm not sure what to do about it. 

The rest of warnings looks more like something we could easily deal with:

```
xloginsert.c:742:18: warning: implicit conversion from 'int' to 'char'
changes value from 253 to -3 [-Wconstant-conversion]
```

Patch that fixes these warnings is attached to this email.

[1] http://lists.llvm.org/pipermail/cfe-dev/2016-March/048126.html

-- 
Best regards,
Aleksander Alekseev
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f23e108628..0007010b25 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -5015,7 +5015,7 @@ BootStrapXLOG(void)
 	record->xl_rmid = RM_XLOG_ID;
 	recptr += SizeOfXLogRecord;
 	/* fill the XLogRecordDataHeaderShort struct */
-	*(recptr++) = XLR_BLOCK_ID_DATA_SHORT;
+	*(recptr++) = (char)XLR_BLOCK_ID_DATA_SHORT;
 	*(recptr++) = sizeof(checkPoint);
 	memcpy(recptr, , sizeof(checkPoint));
 	recptr += sizeof(checkPoint);
diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index 03b05f937f..ea8e915029 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -739,7 +739,7 @@ XLogRecordAssemble(RmgrId rmid, uint8 info,
 	if ((curinsert_flags & XLOG_INCLUDE_ORIGIN) &&
 		replorigin_session_origin != InvalidRepOriginId)
 	{
-		*(scratch++) = XLR_BLOCK_ID_ORIGIN;
+		*(scratch++) = (char)XLR_BLOCK_ID_ORIGIN;
 		memcpy(scratch, _session_origin, sizeof(replorigin_session_origin));
 		scratch += sizeof(replorigin_session_origin);
 	}
@@ -749,13 +749,13 @@ XLogRecordAssemble(RmgrId rmid, uint8 info,
 	{
 		if (mainrdata_len > 255)
 		{
-			*(scratch++) = XLR_BLOCK_ID_DATA_LONG;
+			*(scratch++) = (char)XLR_BLOCK_ID_DATA_LONG;
 			memcpy(scratch, _len, sizeof(uint32));
 			scratch += sizeof(uint32);
 		}
 		else
 		{
-			*(scratch++) = XLR_BLOCK_ID_DATA_SHORT;
+			*(scratch++) = (char)XLR_BLOCK_ID_DATA_SHORT;
 			*(scratch++) = (uint8) mainrdata_len;
 		}
 		rdt_datas_last->next = mainrdata_head;
diff --git a/src/bin/pg_resetwal/pg_resetwal.c b/src/bin/pg_resetwal/pg_resetwal.c
index 96b7097f8b..9165da1ee5 100644
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -1100,7 +1100,7 @@ WriteEmptyXLOG(void)
 	record->xl_rmid = RM_XLOG_ID;
 
 	recptr += SizeOfXLogRecord;
-	*(recptr++) = XLR_BLOCK_ID_DATA_SHORT;
+	*(recptr++) = (char)XLR_BLOCK_ID_DATA_SHORT;
 	*(recptr++) = sizeof(CheckPoint);
 	memcpy(recptr, ,
 		   sizeof(CheckPoint));


signature.asc
Description: PGP signature

Re: [HACKERS] SCRAM authentication, take three

[Aleksander Alekseev]

And a few more things I've noticed after a closer look:

* build_client_first_message does not free `state->client_nonce` if
  second malloc (for `buf`) fails
* same for `state->client_first_message_bare`
* ... and most other procedures declared in fe-auth-scram.c file
  (see malloc and strdup calls)
* scram_Normalize doesn't check malloc return value

Sorry for lots of emails.

On Mon, Feb 20, 2017 at 03:15:14PM +0300, Aleksander Alekseev wrote:
> Speaking about flaws, it looks like there is a memory leak in
> array_to_utf procedure - result is allocated twice.
> 
> On Mon, Feb 20, 2017 at 02:51:13PM +0300, Aleksander Alekseev wrote:
> > Hi!
> > 
> > Currently I don't see any significant flaws in these patches. However I
> > would like to verify that implemented algorithms are compatible with
> > algorithms implemented by third party.
> > 
> > For instance, for user 'eax' and password 'pass' I got the following
> > record in pg_authid:
> > 
> > ```
> > scram-sha-256:
> > xtznkRN/nc/1DQ==:
> > 4096:
> > 2387c124a3139a276b848c910f43ece05dd670d0977ace4f20d724af522312e4:
> > 5e3bdd6584880198b0375acabd8754c460af2389499f71a756660a10a8aaa843
> > ```
> > 
> > Let's say I would like to implement SCRAM in pure Python, for instance
> > add it to pg8000 driver. Firstly I need to know how to generate server
> > key and client key having only user name and password. Somehow like
> > this:
> > 
> > ```
> >  >>> import base64
> >  >>> import hashlib
> >  >>> base64.b16encode(hashlib.pbkdf2_hmac('sha256', b'pass',
> >  ...base64.b64decode(b'xtznkRN/nc/1DQ=='), 4096))
> > b'14B90CFCF690120399A0E6D30C60DD9D9D221CD9C2E31EA0A00514C41823E6C3'
> > ```
> > 
> > Naturally it doesn't work because both SCRAM_SERVER_KEY_NAME and
> > SCRAM_CLIENT_KEY_NAME should also be involved. I see PostgreSQL
> > implementation just in front of me but unfortunately I'm still having
> > problems calculating exactly the same server key and client key. It makes
> > me worry whether PostgreSQL implementation is OK.
> > 
> > Could you please give an example of how to do it?
> > 
> > On Mon, Feb 20, 2017 at 03:29:19PM +0900, Michael Paquier wrote:
> > > On Sun, Feb 19, 2017 at 10:07 PM, Michael Paquier
> > > <michael.paqu...@gmail.com> wrote:
> > > > There is something that I think is still unwelcome in this patch: the
> > > > interface in pg_hba.conf. I mentioned that in the previous thread but
> > > > now if you want to match a user and a database with a scram password
> > > > you need to do that with the current set of patches:
> > > > local $dbname $user scram
> > > > That's not really portable as SCRAM is one protocol in the SASL
> > > > family, and even worse in our case we use SCRAM-SHA-256. I'd like to
> > > > change pg_hba.conf to be as follows:
> > > > local $dbname $user sasl protocol=scram_sha_256
> > > > This is extensible for the future, and protocol is a mandatory option
> > > > that would have now just a single value: scram_sha_256. Heikki,
> > > > others, are you fine with that?
> > > 
> > > I have implemented that as 0009 which is attached, and need to be
> > > applied on the rest of upthread. I am not sure if we want to make the
> > > case where no protocol is specified map to everything. This would be a
> > > tricky support for users in the future if new authentication
> > > mechanisms for SASL are added in the future.
> > > 
> > > Another issue that I have is: do we really want to have
> > > password_encryption being set to "scram" for verifiers of
> > > SCRAM-SHA-256? I would think that scram_sha_256 makes the most sense.
> > > Who knows, perhaps there could be in a couple of years a SHA-SHA-512..
> > > 
> > > At the same time, attached is a new version of 0008 that implements
> > > SASLprep, I have stabilized the beast after fixing some allocation
> > > calculations when converting the decomposed pg_wchar array back to a
> > > utf8 string.
> > > -- 
> > > Michael
> > 
> > -- 
> > Best regards,
> > Aleksander Alekseev
> 
> 
> 
> -- 
> Best regards,
> Aleksander Alekseev



-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] SCRAM authentication, take three

[Aleksander Alekseev]

Speaking about flaws, it looks like there is a memory leak in
array_to_utf procedure - result is allocated twice.

On Mon, Feb 20, 2017 at 02:51:13PM +0300, Aleksander Alekseev wrote:
> Hi!
> 
> Currently I don't see any significant flaws in these patches. However I
> would like to verify that implemented algorithms are compatible with
> algorithms implemented by third party.
> 
> For instance, for user 'eax' and password 'pass' I got the following
> record in pg_authid:
> 
> ```
> scram-sha-256:
> xtznkRN/nc/1DQ==:
> 4096:
> 2387c124a3139a276b848c910f43ece05dd670d0977ace4f20d724af522312e4:
> 5e3bdd6584880198b0375acabd8754c460af2389499f71a756660a10a8aaa843
> ```
> 
> Let's say I would like to implement SCRAM in pure Python, for instance
> add it to pg8000 driver. Firstly I need to know how to generate server
> key and client key having only user name and password. Somehow like
> this:
> 
> ```
>  >>> import base64
>  >>> import hashlib
>  >>> base64.b16encode(hashlib.pbkdf2_hmac('sha256', b'pass',
>  ...base64.b64decode(b'xtznkRN/nc/1DQ=='), 4096))
> b'14B90CFCF690120399A0E6D30C60DD9D9D221CD9C2E31EA0A00514C41823E6C3'
> ```
> 
> Naturally it doesn't work because both SCRAM_SERVER_KEY_NAME and
> SCRAM_CLIENT_KEY_NAME should also be involved. I see PostgreSQL
> implementation just in front of me but unfortunately I'm still having
> problems calculating exactly the same server key and client key. It makes
> me worry whether PostgreSQL implementation is OK.
> 
> Could you please give an example of how to do it?
> 
> On Mon, Feb 20, 2017 at 03:29:19PM +0900, Michael Paquier wrote:
> > On Sun, Feb 19, 2017 at 10:07 PM, Michael Paquier
> > <michael.paqu...@gmail.com> wrote:
> > > There is something that I think is still unwelcome in this patch: the
> > > interface in pg_hba.conf. I mentioned that in the previous thread but
> > > now if you want to match a user and a database with a scram password
> > > you need to do that with the current set of patches:
> > > local $dbname $user scram
> > > That's not really portable as SCRAM is one protocol in the SASL
> > > family, and even worse in our case we use SCRAM-SHA-256. I'd like to
> > > change pg_hba.conf to be as follows:
> > > local $dbname $user sasl protocol=scram_sha_256
> > > This is extensible for the future, and protocol is a mandatory option
> > > that would have now just a single value: scram_sha_256. Heikki,
> > > others, are you fine with that?
> > 
> > I have implemented that as 0009 which is attached, and need to be
> > applied on the rest of upthread. I am not sure if we want to make the
> > case where no protocol is specified map to everything. This would be a
> > tricky support for users in the future if new authentication
> > mechanisms for SASL are added in the future.
> > 
> > Another issue that I have is: do we really want to have
> > password_encryption being set to "scram" for verifiers of
> > SCRAM-SHA-256? I would think that scram_sha_256 makes the most sense.
> > Who knows, perhaps there could be in a couple of years a SHA-SHA-512..
> > 
> > At the same time, attached is a new version of 0008 that implements
> > SASLprep, I have stabilized the beast after fixing some allocation
> > calculations when converting the decomposed pg_wchar array back to a
> > utf8 string.
> > -- 
> > Michael
> 
> -- 
> Best regards,
> Aleksander Alekseev



-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] SCRAM authentication, take three

[Aleksander Alekseev]

Hi!

Currently I don't see any significant flaws in these patches. However I
would like to verify that implemented algorithms are compatible with
algorithms implemented by third party.

For instance, for user 'eax' and password 'pass' I got the following
record in pg_authid:

```
scram-sha-256:
xtznkRN/nc/1DQ==:
4096:
2387c124a3139a276b848c910f43ece05dd670d0977ace4f20d724af522312e4:
5e3bdd6584880198b0375acabd8754c460af2389499f71a756660a10a8aaa843
```

Let's say I would like to implement SCRAM in pure Python, for instance
add it to pg8000 driver. Firstly I need to know how to generate server
key and client key having only user name and password. Somehow like
this:

```
 >>> import base64
 >>> import hashlib
 >>> base64.b16encode(hashlib.pbkdf2_hmac('sha256', b'pass',
 ...base64.b64decode(b'xtznkRN/nc/1DQ=='), 4096))
b'14B90CFCF690120399A0E6D30C60DD9D9D221CD9C2E31EA0A00514C41823E6C3'
```

Naturally it doesn't work because both SCRAM_SERVER_KEY_NAME and
SCRAM_CLIENT_KEY_NAME should also be involved. I see PostgreSQL
implementation just in front of me but unfortunately I'm still having
problems calculating exactly the same server key and client key. It makes
me worry whether PostgreSQL implementation is OK.

Could you please give an example of how to do it?

On Mon, Feb 20, 2017 at 03:29:19PM +0900, Michael Paquier wrote:
> On Sun, Feb 19, 2017 at 10:07 PM, Michael Paquier
> <michael.paqu...@gmail.com> wrote:
> > There is something that I think is still unwelcome in this patch: the
> > interface in pg_hba.conf. I mentioned that in the previous thread but
> > now if you want to match a user and a database with a scram password
> > you need to do that with the current set of patches:
> > local $dbname $user scram
> > That's not really portable as SCRAM is one protocol in the SASL
> > family, and even worse in our case we use SCRAM-SHA-256. I'd like to
> > change pg_hba.conf to be as follows:
> > local $dbname $user sasl protocol=scram_sha_256
> > This is extensible for the future, and protocol is a mandatory option
> > that would have now just a single value: scram_sha_256. Heikki,
> > others, are you fine with that?
> 
> I have implemented that as 0009 which is attached, and need to be
> applied on the rest of upthread. I am not sure if we want to make the
> case where no protocol is specified map to everything. This would be a
> tricky support for users in the future if new authentication
> mechanisms for SASL are added in the future.
> 
> Another issue that I have is: do we really want to have
> password_encryption being set to "scram" for verifiers of
> SCRAM-SHA-256? I would think that scram_sha_256 makes the most sense.
> Who knows, perhaps there could be in a couple of years a SHA-SHA-512..
> 
> At the same time, attached is a new version of 0008 that implements
> SASLprep, I have stabilized the beast after fixing some allocation
> calculations when converting the decomposed pg_wchar array back to a
> utf8 string.
> -- 
> Michael

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] SCRAM authentication, take three

[Aleksander Alekseev]

No, I'm afraid `make distclean` doesn't help. I've re-checked twice.

On Mon, Feb 06, 2017 at 12:52:11PM -0300, Alvaro Herrera wrote:
> Aleksander Alekseev wrote:
> > Hello.
> > 
> > Good to know that the work on this great feature continues!
> > 
> > However this set of patches does not pass `make check-world` on my
> > laptop.
> > 
> > Here is how I build and test PostgreSQL:
> > 
> > https://github.com/afiskon/pgscripts/blob/master/full-build.sh
> 
> I think you need "make distclean" instead of "make clean", unless you
> also add --enable-depend to configure.
> 
> -- 
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] SCRAM authentication, take three

[Aleksander Alekseev]

Hello.

Good to know that the work on this great feature continues!

However this set of patches does not pass `make check-world` on my
laptop.

Here is how I build and test PostgreSQL:

https://github.com/afiskon/pgscripts/blob/master/full-build.sh

Error message:

http://afiskon.ru/s/0b/258c6e4f14_123.txt

regression.diffs:

http://afiskon.ru/s/a0/4993102c32_regression.diffs.txt

regression.out:

http://afiskon.ru/s/4b/acd5a58374_regression.out.txt

Backtrace:

http://afiskon.ru/s/00/c0b32b45a6_bt.txt

I'm using Arch Linux with latest updates, Python version is 3.6.0. I
have no idea what SCRAM has to do with Python, but this issue reproduced
two times of two I did `make check-world`. There is no such issue in
current master branch.

On Mon, Feb 06, 2017 at 02:55:08PM +0200, Heikki Linnakangas wrote:
> I rebased the SCRAM authentication patches over current master. Here you
> are.
> 
> I'm trying to whack this into the final shape that it could actually be
> committed. The previous thread on SCRAM authentication has grown
> ridiculously long and meandered into all kinds of details, so I thought it's
> best to start afresh with a new thread.
> 
> So, if you haven't paid attention on this for a while, now would be a good
> time to have another look at the patch. I believe all the basic
> functionality, documentation, and tests are there, and there are no known
> bugs. Please review! I'll start reading through these myself again tomorrow.
> 
> One thing that's missing, that we need to address before the release, is the
> use of SASLPrep to "normalize" the password. We discussed that in the
> previous thread, and I think we have a good path forward on it. I'd be happy
> to leave that for a follow-up commit, after these other patches have been
> committed, so we can discuss that work separately.
> 
> These are also available on Michael's github repository, at
> https://github.com/michaelpq/postgres/tree/scram.
> 
> - Heikki
> 

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] PATCH: recursive json_populate_record()

[Aleksander Alekseev]

Hello.

I've noticed that this patch is on CF and needs a reviewer so I decided
to take a look. Code looks good to me in general, it's well covered by
tests and passes `make check-world`.

However it would be nice to have a little more comments. In my opinion
every procedure have to have at least a short description - what it
does, what arguments it receives and what it returns, even if it's a
static procedure. Same applies for structures and their fields.

Another thing that bothers me is a FIXME comment:

```
tupdesc = lookup_rowtype_tupdesc(type, typmod); /* FIXME cache */
```

I suggest remove it or implement a caching here as planned.

On Tue, Dec 13, 2016 at 10:07:46AM +0900, Michael Paquier wrote:
> On Tue, Dec 13, 2016 at 9:38 AM, Nikita Glukhov <n.glu...@postgrespro.ru> 
> wrote:
> > It also fixes the following errors/inconsistencies caused by lost quoting of
> > string json values:
> >
> > [master]=# select * from json_to_record('{"js": "a"}') as rec(js json);
> > ERROR:  invalid input syntax for type json
> > DETAIL:  Token "a" is invalid.
> > CONTEXT:  JSON data, line 1: a
> 
> The docs mention that this is based on a best effort now. It may be
> interesting to improve that.
> 
> > [master]=# select * from json_to_record('{"js": "true"}') as rec(js json);
> >   js
> > --
> > true
> >
> > [patched]=# select * from json_to_record('{"js": "a"}') as rec(js json);
> >   js
> > -
> >  "a"
> 
> That's indeed valid JSON.
> 
> > [patched]=# select * from json_to_record('{"js": "true"}') as rec(js json);
> >js
> > 
> >  "true"
> 
> And so is that.
> 
> > The second patch adds assignment of correct ndims to array fields of RECORD
> > function result types.
> > Without this patch, attndims in tuple descriptors of RECORD types is always
> > 0 and the corresponding assertion fails in the next test:
> >
> > [patched]=# select json_to_record('{"a": [1, 2, 3]}') as rec(a int[]);
> >
> >
> > Should I submit these patches to commitfest?
> 
> It seems to me that it would be a good idea to add them.
> -- 
> Michael
> 
> 
> -- 
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] [PATCH] Generic type subscription

[Aleksander Alekseev]

As I mentioned above [1] in my humble opinion this patch is not at all in a
"good shape" until it breaks existing extensions.

[1] http://postgr.es/m/20161115080324.GA5351%40e733.localdomain

On Mon, Dec 26, 2016 at 10:49:30PM +0700, Dmitry Dolgov wrote:
> > On 5 December 2016 at 12:03, Haribabu Kommi <kommi.harib...@gmail.com>
>  wrote:
> 
> > > Moved to next CF with "needs review" status.
> 
> Looks like we stuck here little bit. Does anyone else have any
> suggestions/improvements, or this patch is in good enough shape?

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

[HACKERS] [PATCH] Fix for documentation of timestamp type

[Aleksander Alekseev]

Hello.

Currently doc/src/sgml/datatype.sgml states:

```
When timestamp values are stored as eight-byte integers
(currently the default), microsecond precision is available over 
the full range of values. When timestamp values are
stored as double precision floating-point numbers instead (a
deprecated compile-time option), the effective limit of precision
might be less than 6. timestamp values are stored as
seconds before or after midnight 2000-01-01. [...]
```

It gives a wrong impression that by default timestamp is stored as a
number of seconds after midnight 2000-01-01 in a eight-byte integer. In
fact timestamp is stored in MICROseconds, not seconds. For instance,
2016-12-12 16:03:14.643886 is represented as number 534873794643886:

```
$ echo "select relfilenode from pg_class where relname = 'tst';" | psql
 relfilenode 
-
   16431
(1 row)

$ find /path/to/data/dir -type f -name 16431
[...]

$ hexdump -C path/to/found/table/segment
  00 00 00 00 08 13 10 03  00 00 00 00 1c 00 e0 1f
0010  00 20 04 20 00 00 00 00  e0 9f 40 00 00 00 00 00
0020  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00
*
1fe0  3c 02 00 00 00 00 00 00  00 00 00 00 00 00 00 00
1ff0  01 00 01 00 00 09 18 00  ae 87 87 02 77 e6 01 00

$ python
>>> "{:x}".format(534873794643886)
'1e677028787ae'
```

'ae 87 87 02 77 e6 01 00' is exactly what is physically stored on disk.
You can calculate current year from number 534873794643886 like this:

```
>>> int(2000 + 534873794643886 / 1000 / 1000 / 60 / 60 / 24 / 365.2425)
2016
```

I suggest to rewrite the documentation a bit to make it more clear that
by default timestamp is stored in microseconds. Corresponding patch is
attached.

-- 
Best regards,
Aleksander Alekseev
diff --git a/doc/src/sgml/datatype.sgml b/doc/src/sgml/datatype.sgml
index 67d0c34..01a8492 100644
--- a/doc/src/sgml/datatype.sgml
+++ b/doc/src/sgml/datatype.sgml
@@ -1654,10 +1654,11 @@ SELECT E'\\xDEADBEEF';
 the full range of values. When timestamp values are
 stored as double precision floating-point numbers instead (a
 deprecated compile-time option), the effective limit of precision
-might be less than 6. timestamp values are stored as
-seconds before or after midnight 2000-01-01.  When
+might be less than 6. By default timestamp values are
+storead as microseconds before or after midnight 2000-01-01.  When
 timestamp values are implemented using floating-point
-numbers, microsecond precision is achieved for dates within a few
+numbers, values are storead as number of seconds. In this case
+microsecond precision is achieved for dates within a few
 years of 2000-01-01, but the precision degrades for dates further
 away. Note that using floating-point datetimes allows a larger
 range of timestamp values to be represented than


signature.asc
Description: PGP signature

Re: [HACKERS] [PATCH] Refactor "if(strspn(str, ...) == strlen(str)" code

[Aleksander Alekseev]

Tom, Geoff,

Thanks for your feedback! Here is version 2 of this patch.

> You could just change it to
> if (str[strspn(str, " \t\n\r\f")] == '\0')
> to mitigate calling strlen. It's safe to do so because strspn will
> only return values from 0 to strlen(str).

> [...] I have serious doubts that the "optimized" implementation
> you propose is actually faster than a naive one; it may be slower, and
> it's certainly longer and harder to understand/test.

I would like to point out that I never said it's optimized. However I
like the code Geoff proposed. It definitely doesn't make anything worse.
I decided to keep pg_str_contansonly procedure (i.e. not to inline this
code) for now. Code with strspn looks OK in a simple example. However in
a concrete context it looks like a bad Perl code in ROT13 to me:

```
/* try to figure out what's exactly going on */
if(somelongname[strspn(somelongname /* yes, again */, "longlistofchars")] != 
'\0')
```
> Function name seems weirdly spelled.

I named it the same way pg_str_endswith is named. However I'm open for
better suggestions here.

> Whether it's worth worrying about, I dunno.  This is hardly the only
> C idiom you need to be familiar with to read the PG code.

Well, at least this v2 version of the patch removes second string
scanning. And I still believe that this inlined strspn is not readable
or obvious at all.

-- 
Best regards,
Aleksander Alekseev
diff --git a/src/backend/parser/parse_type.c b/src/backend/parser/parse_type.c
index a8bb472..a1c5853 100644
--- a/src/backend/parser/parse_type.c
+++ b/src/backend/parser/parse_type.c
@@ -17,6 +17,7 @@
 #include "access/htup_details.h"
 #include "catalog/namespace.h"
 #include "catalog/pg_type.h"
+#include "common/string.h"
 #include "lib/stringinfo.h"
 #include "nodes/makefuncs.h"
 #include "parser/parser.h"
@@ -696,7 +697,7 @@ typeStringToTypeName(const char *str)
 	ErrorContextCallback ptserrcontext;
 
 	/* make sure we give useful error for empty input */
-	if (strspn(str, " \t\n\r\f") == strlen(str))
+	if (pg_str_containsonly(str, " \t\n\r\f"))
 		goto fail;
 
 	initStringInfo();
diff --git a/src/backend/tsearch/ts_utils.c b/src/backend/tsearch/ts_utils.c
index 9c3e15c..81d6132 100644
--- a/src/backend/tsearch/ts_utils.c
+++ b/src/backend/tsearch/ts_utils.c
@@ -17,6 +17,7 @@
 #include 
 
 #include "miscadmin.h"
+#include "common/string.h"
 #include "tsearch/ts_locale.h"
 #include "tsearch/ts_utils.h"
 
@@ -45,7 +46,7 @@ get_tsearch_config_filename(const char *basename,
 	 * and case-insensitive filesystems, and non-ASCII characters create other
 	 * interesting risks, so on the whole a tight policy seems best.
 	 */
-	if (strspn(basename, "abcdefghijklmnopqrstuvwxyz0123456789_") != strlen(basename))
+	if (!pg_str_containsonly(basename, "abcdefghijklmnopqrstuvwxyz0123456789_"))
 		ereport(ERROR,
 (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
  errmsg("invalid text search configuration file name \"%s\"",
diff --git a/src/backend/utils/adt/regproc.c b/src/backend/utils/adt/regproc.c
index 394042c..7e41359 100644
--- a/src/backend/utils/adt/regproc.c
+++ b/src/backend/utils/adt/regproc.c
@@ -32,6 +32,7 @@
 #include "catalog/pg_ts_config.h"
 #include "catalog/pg_ts_dict.h"
 #include "catalog/pg_type.h"
+#include "common/string.h"
 #include "lib/stringinfo.h"
 #include "miscadmin.h"
 #include "parser/parse_type.h"
@@ -75,7 +76,7 @@ regprocin(PG_FUNCTION_ARGS)
 	/* Numeric OID? */
 	if (pro_name_or_oid[0] >= '0' &&
 		pro_name_or_oid[0] <= '9' &&
-		strspn(pro_name_or_oid, "0123456789") == strlen(pro_name_or_oid))
+		pg_str_containsonly(pro_name_or_oid, "0123456789"))
 	{
 		result = DatumGetObjectId(DirectFunctionCall1(oidin,
 		  CStringGetDatum(pro_name_or_oid)));
@@ -286,7 +287,7 @@ regprocedurein(PG_FUNCTION_ARGS)
 	/* Numeric OID? */
 	if (pro_name_or_oid[0] >= '0' &&
 		pro_name_or_oid[0] <= '9' &&
-		strspn(pro_name_or_oid, "0123456789") == strlen(pro_name_or_oid))
+		pg_str_containsonly(pro_name_or_oid, "0123456789"))
 	{
 		result = DatumGetObjectId(DirectFunctionCall1(oidin,
 		  CStringGetDatum(pro_name_or_oid)));
@@ -535,7 +536,7 @@ regoperin(PG_FUNCTION_ARGS)
 	/* Numeric OID? */
 	if (opr_name_or_oid[0] >= '0' &&
 		opr_name_or_oid[0] <= '9' &&
-		strspn(opr_name_or_oid, "0123456789") == strlen(opr_name_or_oid))
+		pg_str_containsonly(opr_name_or_oid, "0123456789"))
 	{
 		result = DatumGetObjectId(DirectFunctionCall1(oidin,
 		  CStringGetDatum(opr_name_or_oid)));
@@ -750,7 +751,7 @@ regoperatorin(PG_FUNCTION_ARGS)
 	/* Numeric OID? */
 	if (opr_

[HACKERS] [PATCH] Refactor "if(strspn(str, ...) == strlen(str)" code

[Aleksander Alekseev]

Hi.

I noticed that there is a lot of repeating code like this:

```
if (strspn(str, " \t\n\r\f") == strlen(str))
```

I personally don't find it particularly readable, not mentioning that
traversing a string twice doesn't look as a good idea (you can check
using objdump that latest GCC 6.2 doesn't optimize this code).

How about rewriting such a code like this?

```
if (pg_str_containsonly(str, " \t\n\r\f"))
```

Corresponding patch is attached. I don't claim that my implementation of
pg_str_containsonly procedure is faster that strspn + strlen, but at
least such refactoring makes code a little more readable.

-- 
Best regards,
Aleksander Alekseev
diff --git a/src/backend/parser/parse_type.c b/src/backend/parser/parse_type.c
index a8bb472..a1c5853 100644
--- a/src/backend/parser/parse_type.c
+++ b/src/backend/parser/parse_type.c
@@ -17,6 +17,7 @@
 #include "access/htup_details.h"
 #include "catalog/namespace.h"
 #include "catalog/pg_type.h"
+#include "common/string.h"
 #include "lib/stringinfo.h"
 #include "nodes/makefuncs.h"
 #include "parser/parser.h"
@@ -696,7 +697,7 @@ typeStringToTypeName(const char *str)
 	ErrorContextCallback ptserrcontext;
 
 	/* make sure we give useful error for empty input */
-	if (strspn(str, " \t\n\r\f") == strlen(str))
+	if (pg_str_containsonly(str, " \t\n\r\f"))
 		goto fail;
 
 	initStringInfo();
diff --git a/src/backend/tsearch/ts_utils.c b/src/backend/tsearch/ts_utils.c
index 9c3e15c..81d6132 100644
--- a/src/backend/tsearch/ts_utils.c
+++ b/src/backend/tsearch/ts_utils.c
@@ -17,6 +17,7 @@
 #include 
 
 #include "miscadmin.h"
+#include "common/string.h"
 #include "tsearch/ts_locale.h"
 #include "tsearch/ts_utils.h"
 
@@ -45,7 +46,7 @@ get_tsearch_config_filename(const char *basename,
 	 * and case-insensitive filesystems, and non-ASCII characters create other
 	 * interesting risks, so on the whole a tight policy seems best.
 	 */
-	if (strspn(basename, "abcdefghijklmnopqrstuvwxyz0123456789_") != strlen(basename))
+	if (!pg_str_containsonly(basename, "abcdefghijklmnopqrstuvwxyz0123456789_"))
 		ereport(ERROR,
 (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
  errmsg("invalid text search configuration file name \"%s\"",
diff --git a/src/backend/utils/adt/regproc.c b/src/backend/utils/adt/regproc.c
index 394042c..7e41359 100644
--- a/src/backend/utils/adt/regproc.c
+++ b/src/backend/utils/adt/regproc.c
@@ -32,6 +32,7 @@
 #include "catalog/pg_ts_config.h"
 #include "catalog/pg_ts_dict.h"
 #include "catalog/pg_type.h"
+#include "common/string.h"
 #include "lib/stringinfo.h"
 #include "miscadmin.h"
 #include "parser/parse_type.h"
@@ -75,7 +76,7 @@ regprocin(PG_FUNCTION_ARGS)
 	/* Numeric OID? */
 	if (pro_name_or_oid[0] >= '0' &&
 		pro_name_or_oid[0] <= '9' &&
-		strspn(pro_name_or_oid, "0123456789") == strlen(pro_name_or_oid))
+		pg_str_containsonly(pro_name_or_oid, "0123456789"))
 	{
 		result = DatumGetObjectId(DirectFunctionCall1(oidin,
 		  CStringGetDatum(pro_name_or_oid)));
@@ -286,7 +287,7 @@ regprocedurein(PG_FUNCTION_ARGS)
 	/* Numeric OID? */
 	if (pro_name_or_oid[0] >= '0' &&
 		pro_name_or_oid[0] <= '9' &&
-		strspn(pro_name_or_oid, "0123456789") == strlen(pro_name_or_oid))
+		pg_str_containsonly(pro_name_or_oid, "0123456789"))
 	{
 		result = DatumGetObjectId(DirectFunctionCall1(oidin,
 		  CStringGetDatum(pro_name_or_oid)));
@@ -535,7 +536,7 @@ regoperin(PG_FUNCTION_ARGS)
 	/* Numeric OID? */
 	if (opr_name_or_oid[0] >= '0' &&
 		opr_name_or_oid[0] <= '9' &&
-		strspn(opr_name_or_oid, "0123456789") == strlen(opr_name_or_oid))
+		pg_str_containsonly(opr_name_or_oid, "0123456789"))
 	{
 		result = DatumGetObjectId(DirectFunctionCall1(oidin,
 		  CStringGetDatum(opr_name_or_oid)));
@@ -750,7 +751,7 @@ regoperatorin(PG_FUNCTION_ARGS)
 	/* Numeric OID? */
 	if (opr_name_or_oid[0] >= '0' &&
 		opr_name_or_oid[0] <= '9' &&
-		strspn(opr_name_or_oid, "0123456789") == strlen(opr_name_or_oid))
+		pg_str_containsonly(opr_name_or_oid, "0123456789"))
 	{
 		result = DatumGetObjectId(DirectFunctionCall1(oidin,
 		  CStringGetDatum(opr_name_or_oid)));
@@ -995,7 +996,7 @@ regclassin(PG_FUNCTION_ARGS)
 	/* Numeric OID? */
 	if (class_name_or_oid[0] >= '0' &&
 		class_name_or_oid[0] <= '9' &&
-		strspn(class_name_or_oid, "0123456789") == strlen(class_name_or_oid))
+		pg_str_containsonly(class_name_or_oid, "0123456789"))
 	{
 		result = DatumGetObjectId(DirectFunctionCall1(oidin,
 		CStringGetDatum(class_name_or_oid)));
@@ -1186,7 +1187,7 @@ regtypein(PG_FUNCTION_ARGS)

[HACKERS] Re: [PATCH] PostgresNode.pm enhancements, pg_lsn helper, and some more recovery tests

[Aleksander Alekseev]

Hello, Craig.

I noticed that these patches have a "Needs review" status and decided to
take a look. Surprisingly I didn't manage to find many defects.

* I like this idea in general;
* Code is test covered and documented well;
* All tests pass;
* No warnings during compilation and/or tests run;
* I didn't find any typos;
* Code style seems to be OK;

Though are you sure you want to name a class like_this instead of LikeThis?
It's quite uncommon in Perl code and usually used only for packages like
"strict" and "warnings". However I prefer no to insist on changes like
this since it's a matter of taste.

The bottom line --- this code looks good to me. If there is nothing you
would like to change in the last moment I suggest to change the status
to "Ready for committer".

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Test "tablespace" fails during `make installcheck` on master-replica setup

[Aleksander Alekseev]

> In the same host, primary and standby will try to use the tablespace
> in the same path. That's the origin of this breakage.

Sorry, I don't follow. Don't master and replica use different
directories to store _all_ data? Particularly in my case:

```
$ find path/to/postgresql-install/ -type d -name pg_tblspc
/home/eax/work/postgrespro/postgresql-install/data-slave/pg_tblspc
/home/eax/work/postgrespro/postgresql-install/data-master/pg_tblspc
```

Where exactly a collision happens?

On Wed, Dec 07, 2016 at 09:39:20PM +0900, Michael Paquier wrote:
> On Wed, Dec 07, 2016 at 03:18:59PM +0300, Aleksander Alekseev wrote:
> > I noticed, that `make installcheck` fails on my laptop with following
> > errors:
> > 
> > http://afiskon.ru/s/98/6f94ce2cfa_regression.out.txt
> > http://afiskon.ru/s/b3/d0da05597e_regression.diffs.txt
> 
> The interesting bit for the archives:
> 
> *** 
> /home/eax/work/postgrespro/postgresql-src/src/test/regress/expected/tablespace.out
> 2016-12-07 13:53:44.000728436 +0300
> --- 
> /home/eax/work/postgrespro/postgresql-src/src/test/regress/results/tablespace.out
>  2016-12-07 13:53:46.150728558 +0300
> ***
> *** 66,71 
> --- 66,72 
> INSERT INTO testschema.test_default_tab VALUES (1);
> CREATE INDEX test_index1 on testschema.test_default_tab (id);
> CREATE INDEX test_index2 on testschema.test_default_tab (id) TABLESPACE 
> regress_tblspace;
> + ERROR:  could not open file "pg_tblspc/16395/PG_10_201612061/16393/16407": 
> No such file or directory
> \d testschema.test_index1
> 
> > Any ideas what can cause this issue?
> 
> In the same host, primary and standby will try to use the tablespace
> in the same path. That's the origin of this breakage.
> -- 
> Michael



-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

[HACKERS] Test "tablespace" fails during `make installcheck` on master-replica setup

[Aleksander Alekseev]

Hello.

I noticed, that `make installcheck` fails on my laptop with following
errors:

http://afiskon.ru/s/98/6f94ce2cfa_regression.out.txt
http://afiskon.ru/s/b3/d0da05597e_regression.diffs.txt

My first idea was to use `git bisect`. It turned out that this issue
reproduces on commits back from 2015 as well (older versions don't compile
on my laptop). However it reproduces rarely and with different errors:

http://afiskon.ru/s/8e/1ad2c8ed2b_regression.diffs-8c48375.txt

Here are scripts I use to compile and test PostgreSQL:

https://github.com/afiskon/pgscripts

Exact steps to reproduce are:

```
./quick-build.sh && ./install.sh && make installcheck
```

Completely removing all `configure` flags doesn't make any difference.
Issue reproduces only on master-replica setup i.e. if instead of
install.sh you run ./single-install.sh all tests pass.

I'm using Arch Linux and GCC 6.2.1.

Any ideas what can cause this issue?

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] [PATCH] Generic type subscription

[Aleksander Alekseev]

Hello.

I took a look on the latest -v4 patch. I would like to note that this
patch breaks a backward compatibility. For instance sr_plan extension[1]
stop to compile with errors like:

```
serialize.c:38:2: error: unknown type name ‘ArrayRef’
  JsonbValue *ArrayRef_ser(const ArrayRef *node, JsonbParseState *state,
bool sub_object);
  ^
serialize.c:913:2: error: unknown type name ‘ArrayRef’
  JsonbValue *ArrayRef_ser(const ArrayRef *node, JsonbParseState *state,
bool sub_object)
  ^
In file included from sr_plan.h:4:0,
 from serialize.c:1:

...

```

Other extensions could be affected as well. I'm not saying that it's a
fatal drawback, but it's definitely something you should be aware of.

I personally strongly believe that we shouldn't break extensions between
major releases or at least make it trivial to properly rewrite them.
Unfortunately it's not a case currently.

[1] https://github.com/postgrespro/sr_plan

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

[HACKERS] File content logging during execution of COPY queries (was: Better logging of COPY queries if log_statement='all')

[Aleksander Alekseev]

> > > According to my colleagues it would be very nice to have this feature.
> > > For instance, if you are trying to optimize PostgreSQL for application
> > > that uses COPY and you don't have access to or something like this. 
> > > It could also be useful in some other cases.
> > 
> > This use-case doesn't really make much sense to me.  Can you explain it
> > in more detail?  Is the goal here to replicate all of the statements
> > that are changing data in the database?
> 
> The idea is to record application workload in real environment and write
> a benchmark based on this record. Then using this benchmark we could try
> different OS/DBMS configuration (or maybe hardware), find an extremum,
> then change configuration in production environment.
> 
> It's not always possible to change an application or even database (e.g.
> to use triggers) for this purpose. For instance, if DBMS is provided as
> a service.
> 
> Currently PostgreSQL allows to record all workload _except_ COPY
> queries. Considering how easily it could be done I think it's wrong.
> Basically the only real question here is how it should look like in
> postgresql.conf.

OK, how about introducing a new boolean parameter named log_copy?
Corresponding patch is attached.

-- 
Best regards,
Aleksander Alekseev
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 8c25b45..84a7542 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -5205,6 +5205,20 @@ FROM pg_stat_activity;
   
  
 
+ 
+  log_copy (boolean)
+  
+   log_copy configuration parameter
+  
+  
+  
+   
+Controls whether file content is logged during execution of
+COPY queries.  The default is off.
+   
+  
+ 
+
  
   log_replication_commands (boolean)
   
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 5947e72..1863e27 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -331,6 +331,38 @@ static bool CopyGetInt16(CopyState cstate, int16 *val);
 
 
 /*
+ * Logs file content during COPY ... FROM / COPY ... TO execution if
+ * log_copy = 'on'.
+ */
+static void
+CopyLogStatement(const char* str, bool flush)
+{
+	static StringInfo logString = NULL;
+
+	if(log_copy == false)
+		return;
+
+	if(logString == NULL)
+	{
+		MemoryContext oldctx = MemoryContextSwitchTo(TopMemoryContext);
+		logString = makeStringInfo();
+		MemoryContextSwitchTo(oldctx);
+	}
+
+	appendStringInfoString(logString, str);
+
+	if(flush)
+	{
+		ereport(LOG,
+(errmsg("statement: %s", logString->data),
+ errhidestmt(true),
+ errhidecontext(true)));
+
+		resetStringInfo(logString);
+	}
+}
+
+/*
  * Send copy start/stop messages for frontend copies.  These have changed
  * in past protocol redesigns.
  */
@@ -2045,14 +2077,20 @@ CopyOneRowTo(CopyState cstate, Oid tupleOid, Datum *values, bool *nulls)
 		if (!cstate->binary)
 		{
 			if (need_delim)
+			{
 CopySendChar(cstate, cstate->delim[0]);
+CopyLogStatement(cstate->delim, false);
+			}
 			need_delim = true;
 		}
 
 		if (isnull)
 		{
 			if (!cstate->binary)
+			{
 CopySendString(cstate, cstate->null_print_client);
+CopyLogStatement(cstate->null_print_client, false);
+			}
 			else
 CopySendInt32(cstate, -1);
 		}
@@ -2062,6 +2100,9 @@ CopyOneRowTo(CopyState cstate, Oid tupleOid, Datum *values, bool *nulls)
 			{
 string = OutputFunctionCall(_functions[attnum - 1],
 			value);
+
+CopyLogStatement(string, false);
+
 if (cstate->csv_mode)
 	CopyAttributeOutCSV(cstate, string,
 		cstate->force_quote_flags[attnum - 1],
@@ -2083,6 +2124,7 @@ CopyOneRowTo(CopyState cstate, Oid tupleOid, Datum *values, bool *nulls)
 	}
 
 	CopySendEndOfRow(cstate);
+	CopyLogStatement("", true);
 
 	MemoryContextSwitchTo(oldcontext);
 }
@@ -2914,6 +2956,8 @@ NextCopyFromRawFields(CopyState cstate, char ***fields, int *nfields)
 	if (done && cstate->line_buf.len == 0)
 		return false;
 
+	CopyLogStatement(cstate->line_buf.data, true);
+
 	/* Parse the line into de-escaped field values */
 	if (cstate->csv_mode)
 		fldct = CopyReadAttributesCSV(cstate);
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index bc9d33f..0f035ac 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -415,6 +415,8 @@ bool		log_planner_stats = false;
 bool		log_executor_stats = false;
 bool		log_statement_stats = false;		/* this is sort of all three
  * above together */
+bool		log_copy = false;
+
 bool		log_btree_build_stats = false;
 char	   *event_source;
 
@@ -1161,6 +1163,15 @@ static struct config_bool ConfigureNamesBool[] =
 		false,
 		check_log_stats, NULL, NULL
 	},
+	{
+		{"log_copy", PGC_SUSET, STATS_MONITORING,
+			gettext_noop(&qu

Re: [HACKERS] [PATCH] Better logging of COPY queries if log_statement='all'

[Aleksander Alekseev]

> > According to my colleagues it would be very nice to have this feature.
> > For instance, if you are trying to optimize PostgreSQL for application
> > that uses COPY and you don't have access to or something like this. 
> > It could also be useful in some other cases.
> 
> This use-case doesn't really make much sense to me.  Can you explain it
> in more detail?  Is the goal here to replicate all of the statements
> that are changing data in the database?

The idea is to record application workload in real environment and write
a benchmark based on this record. Then using this benchmark we could try
different OS/DBMS configuration (or maybe hardware), find an extremum,
then change configuration in production environment.

It's not always possible to change an application or even database (e.g.
to use triggers) for this purpose. For instance, if DBMS is provided as
a service.

Currently PostgreSQL allows to record all workload _except_ COPY
queries. Considering how easily it could be done I think it's wrong.
Basically the only real question here is how it should look like in
postgresql.conf.

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] [PATCH] Better logging of COPY queries if log_statement='all'

[Aleksander Alekseev]

> > I'm not in favor of this, especially if it's not even optional. 
> 
> I'm not either.  It sounds good when you're looking at toy examples,
> but not when it means repeating gigabytes of COPY data into the log.

I understand your concern. Perhaps we could create and additional
parameter for enabling/disabling this feature or a new log_statement
value, or maybe both - i.e. rename log_statement and add a new possible
value?

According to my colleagues it would be very nice to have this feature.
For instance, if you are trying to optimize PostgreSQL for application
that uses COPY and you don't have access to or something like this. 
It could also be useful in some other cases.

This feature is very simple and easy to maintain. I'm sure we could find
a solution that will make happy both developers and users. 

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

[HACKERS] [PATCH] Better logging of COPY queries if log_statement='all'

[Aleksander Alekseev]

Hello.

Sometimes it's useful to log content of files used in COPY ... TO ... and
COPY ... FROM ... queries. Unfortunately PostgreSQL doesn't allow to do
it, even if log_statement='all'. Suggested patch fixes this.

Log example:

```
LOG:  statement: create table test (k int, v text);
LOG:  statement: insert into test values (111, 'aaa'), (222, 'bbb');
LOG:  statement: copy test to '/tmp/copy.txt';
LOG:  statement: 111aaa
LOG:  statement: 222bbb
LOG:  statement: delete from test;
LOG:  statement: copy test from '/tmp/copy.txt';
LOG:  statement: 111aaa
LOG:  statement: 222bbb
```
-- 
Best regards,
Aleksander Alekseev
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 5947e72..82b3a18 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -331,6 +331,38 @@ static bool CopyGetInt16(CopyState cstate, int16 *val);
 
 
 /*
+ * Logs file content during COPY ... FROM / COPY ... TO execution if
+ * log_statement = 'all'.
+ */
+static void
+CopyLogStatement(const char* str, bool flush)
+{
+	static StringInfo logString = NULL;
+
+	if(log_statement != LOGSTMT_ALL)
+		return;
+
+	if(logString == NULL)
+	{
+		MemoryContext oldctx = MemoryContextSwitchTo(TopMemoryContext);
+		logString = makeStringInfo();
+		MemoryContextSwitchTo(oldctx);
+	}
+
+	appendStringInfoString(logString, str);
+
+	if(flush)
+	{
+		ereport(LOG,
+(errmsg("statement: %s", logString->data),
+ errhidestmt(true),
+ errhidecontext(true)));
+
+		resetStringInfo(logString);
+	}
+}
+
+/*
  * Send copy start/stop messages for frontend copies.  These have changed
  * in past protocol redesigns.
  */
@@ -2045,14 +2077,20 @@ CopyOneRowTo(CopyState cstate, Oid tupleOid, Datum *values, bool *nulls)
 		if (!cstate->binary)
 		{
 			if (need_delim)
+			{
 CopySendChar(cstate, cstate->delim[0]);
+CopyLogStatement(cstate->delim, false);
+			}
 			need_delim = true;
 		}
 
 		if (isnull)
 		{
 			if (!cstate->binary)
+			{
 CopySendString(cstate, cstate->null_print_client);
+CopyLogStatement(cstate->null_print_client, false);
+			}
 			else
 CopySendInt32(cstate, -1);
 		}
@@ -2062,6 +2100,9 @@ CopyOneRowTo(CopyState cstate, Oid tupleOid, Datum *values, bool *nulls)
 			{
 string = OutputFunctionCall(_functions[attnum - 1],
 			value);
+
+CopyLogStatement(string, false);
+
 if (cstate->csv_mode)
 	CopyAttributeOutCSV(cstate, string,
 		cstate->force_quote_flags[attnum - 1],
@@ -2083,6 +2124,7 @@ CopyOneRowTo(CopyState cstate, Oid tupleOid, Datum *values, bool *nulls)
 	}
 
 	CopySendEndOfRow(cstate);
+	CopyLogStatement("", true);
 
 	MemoryContextSwitchTo(oldcontext);
 }
@@ -2914,6 +2956,8 @@ NextCopyFromRawFields(CopyState cstate, char ***fields, int *nfields)
 	if (done && cstate->line_buf.len == 0)
 		return false;
 
+	CopyLogStatement(cstate->line_buf.data, true);
+
 	/* Parse the line into de-escaped field values */
 	if (cstate->csv_mode)
 		fldct = CopyReadAttributesCSV(cstate);


signature.asc
Description: PGP signature

[HACKERS] [PATCH] pg_filedump is broken

[Aleksander Alekseev]

Hello.

First patch fixes:

```
pg_filedump.c: In function ‘FormatItem’:
pg_filedump.c:994:18: error: ‘SizeOfIptrData’ undeclared (first use in
  this function)
   if (numBytes < SizeOfIptrData)
```

After 8023b582 there is no more SizeOfIptrData macro.

Second patch fixes Makefile. On some systems (notably FreeBSD) there is
no `gcc` by default. Using `cc` is a more crossplatform way to compile a
project.

-- 
Best regards,
Aleksander Alekseev
diff --git a/pg_filedump.c b/pg_filedump.c
index 2f2cd53..ba55711 100644
--- a/pg_filedump.c
+++ b/pg_filedump.c
@@ -991,7 +991,7 @@ FormatItem(unsigned int numBytes, unsigned int startIndex,
 	if (formatAs == ITEM_INDEX)
 	{
 		/* It is an IndexTuple item, so dump the index header */
-		if (numBytes < SizeOfIptrData)
+		if (numBytes < sizeof(ItemPointerData))
 		{
 			if (numBytes)
 			{
diff --git a/Makefile b/Makefile
index 29c1057..b30dbec 100644
--- a/Makefile
+++ b/Makefile
@@ -3,7 +3,7 @@
 # note this must match version macros in pg_filedump.h
 FD_VERSION=9.6.0
 
-CC=gcc
+CC=cc
 CFLAGS=-g -O -Wall -Wmissing-prototypes -Wmissing-declarations
 
 # If working with a PG source directory, point PGSQL_INCLUDE_DIR to its


signature.asc
Description: PGP signature

[HACKERS] [PATCH] Refactoring: rename md5Salt to pwsalt

[Aleksander Alekseev]

Hello.

Since there are plans/efforts to introduce additional authorization
methods in nearest feature I suggest to refactor the code so it
wouldn't mention md5 when it possible. `md5Salt` for instance could be
not only "md5 salt" but also "sha2 salt", etc - depending on what
authorization method was chosen.

Suggested patch (first of many, I hope) renames `md5Salt` to more
general `pwsalt`.

Does it sound reasonable?

-- 
Best regards,
Aleksander Alekseev
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 0ba8530..25bb4c2 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -536,7 +536,7 @@ ClientAuthentication(Port *port)
 		(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
 		 errmsg("MD5 authentication is not supported when \"db_user_namespace\" is enabled")));
 			/* include the salt to use for computing the response */
-			sendAuthRequest(port, AUTH_REQ_MD5, port->md5Salt, 4);
+			sendAuthRequest(port, AUTH_REQ_MD5, port->pwsalt, 4);
 			status = recv_and_check_password_packet(port, );
 			break;
 
diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
index d84a180..98f3315 100644
--- a/src/backend/libpq/crypt.c
+++ b/src/backend/libpq/crypt.c
@@ -96,8 +96,8 @@ md5_crypt_verify(const Port *port, const char *role, char *client_pass,
 			{
 /* stored password already encrypted, only do salt */
 if (!pg_md5_encrypt(shadow_pass + strlen("md5"),
-	port->md5Salt,
-	sizeof(port->md5Salt), crypt_pwd))
+	port->pwsalt,
+	sizeof(port->pwsalt), crypt_pwd))
 {
 	pfree(crypt_pwd);
 	return STATUS_ERROR;
@@ -118,8 +118,8 @@ md5_crypt_verify(const Port *port, const char *role, char *client_pass,
 	return STATUS_ERROR;
 }
 if (!pg_md5_encrypt(crypt_pwd2 + strlen("md5"),
-	port->md5Salt,
-	sizeof(port->md5Salt),
+	port->pwsalt,
+	sizeof(port->pwsalt),
 	crypt_pwd))
 {
 	pfree(crypt_pwd);
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 0c0a609..b7ab8dd 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -2350,7 +2350,7 @@ ConnCreate(int serverFd)
 	 * after.  Else the postmaster's random sequence won't get advanced, and
 	 * all backends would end up using the same salt...
 	 */
-	RandomSalt(port->md5Salt, sizeof(port->md5Salt));
+	RandomSalt(port->pwsalt, sizeof(port->pwsalt));
 
 	/*
 	 * Allocate GSSAPI specific state struct
diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index b91eca5..6b7935c 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -144,7 +144,7 @@ typedef struct Port
 	 * Information that needs to be held during the authentication cycle.
 	 */
 	HbaLine*hba;
-	char		md5Salt[4];		/* Password salt */
+	char		pwsalt[4];		/* Password salt */
 
 	/*
 	 * Information that really has no business at all being in struct Port,
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index 404bc93..9123d5b 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -522,8 +522,8 @@ pg_password_sendauth(PGconn *conn, const char *password, AuthRequest areq)
 	free(crypt_pwd);
 	return STATUS_ERROR;
 }
-if (!pg_md5_encrypt(crypt_pwd2 + strlen("md5"), conn->md5Salt,
-	sizeof(conn->md5Salt), crypt_pwd))
+if (!pg_md5_encrypt(crypt_pwd2 + strlen("md5"), conn->pwsalt,
+	sizeof(conn->pwsalt), crypt_pwd))
 {
 	free(crypt_pwd);
 	return STATUS_ERROR;
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index f3a9e5a..7529fd5 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -2441,8 +2441,8 @@ keep_going:		/* We will come back to here until there is
 /* Get the password salt if there is one. */
 if (areq == AUTH_REQ_MD5)
 {
-	if (pqGetnchar(conn->md5Salt,
-   sizeof(conn->md5Salt), conn))
+	if (pqGetnchar(conn->pwsalt,
+   sizeof(conn->pwsalt), conn))
 	{
 		/* We'll come back when there are more data */
 		return PGRES_POLLING_READING;
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index be6c370..1e18688 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -389,7 +389,7 @@ struct pg_conn
 	/* Miscellaneous stuff */
 	int			be_pid;			/* PID of backend --- needed for cancels */
 	int			be_key;			/* key of backend --- needed for cancels */
-	char		md5Salt[4];		/* password salt received from backend */
+	char		pwsalt[4];		/* password salt received from backend */
 	pgParameterStatus *pstatus; /* ParameterStatus data */
 	int			client_encoding;	/* encoding id */
 	bool		std_strings;	/* standard_conforming_strings */


pgpHAHfrE45d_.pgp
Description: OpenPGP digital signature

Re: [HACKERS] [PATCH] Remove redundant if clause in standbydesc.c

[Aleksander Alekseev]

> > Very simple small patch - see attachment.
> 
>  /* not expected, but print something anyway */
>  else if (msg->id == SHAREDINVALRELMAP_ID)
> -appendStringInfoString(buf, " relmap");
> -else if (msg->id == SHAREDINVALRELMAP_ID)
>  appendStringInfo(buf, " relmap db %u", msg->rm.dbId);
> 
> Looking at inval.c, dbId can be InvalidOid.

Frankly I'm not very familiar with this part of code. InvalidOid is just
zero. Does it create some problem in this case?

-- 
Best regards,
Aleksander Alekseev


-- 
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] Remove redundant if clause in standbydesc.c

[Aleksander Alekseev]

Hello.

Very simple small patch - see attachment.

-- 
Best regards,
Aleksander Alekseev
diff --git a/src/backend/access/rmgrdesc/standbydesc.c b/src/backend/access/rmgrdesc/standbydesc.c
index 13797a3..77983e5 100644
--- a/src/backend/access/rmgrdesc/standbydesc.c
+++ b/src/backend/access/rmgrdesc/standbydesc.c
@@ -122,8 +122,6 @@ standby_desc_invalidations(StringInfo buf,
 			appendStringInfoString(buf, " smgr");
 		/* not expected, but print something anyway */
 		else if (msg->id == SHAREDINVALRELMAP_ID)
-			appendStringInfoString(buf, " relmap");
-		else if (msg->id == SHAREDINVALRELMAP_ID)
 			appendStringInfo(buf, " relmap db %u", msg->rm.dbId);
 		else if (msg->id == SHAREDINVALSNAPSHOT_ID)
 			appendStringInfo(buf, " snapshot %u", msg->sn.relId);

-- 
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] get_home_path: use HOME

[Aleksander Alekseev]

> I work in an environment, where servers are administered by people
> with different user names and identical uid (0).

Multiple users with same uid is orthodox indeed. Just out of curiosity -
what environment is this, if it's not a secret?

> The usage of HOME environment variable (if set) is IMO the right,
> standard and faster way to get_home_path().

As a side note I personally think that considering $HOME environment
variable is not such a bad idea. However I think we should make sure
first that this is really a bug that is relatively easy to reproduce in
real-world environments, a not just a hack for single misconfigured
system.

-- 
Best regards,
Aleksander Alekseev


-- 
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: Implement failover on libpq connect level.

[Aleksander Alekseev]

> Hello, Victor.
> 
> > I'm sending new version of patch.
> > 
> > I've replaced readonly option with target_server_type (with JDBC
> > compatibility alias targetServerType), 
> > 
> > use logic of setting defaults based on number of hosts in the connect
> > string instead of complicated condition in the state machine,
> > 
> > added checks for return values of memory allocation function.
> > 
> > Also, I've solved pg_usleep problem by linking pgsleep.c into libpq
> > along with some other libpgport objects which are already linked there.
> > 
> > Thus client applications don't need to link with libpgport and libpq
> > shared library is self-containted.
> 
> This patch doesn't apply to master branch:
> 
> ```
> $ git apply ~/temp/libpq-failover-8.patch
> /home/eax/temp/libpq-failover-8.patch:184: trailing whitespace.
> check: 
> /home/eax/temp/libpq-failover-8.patch:252: trailing whitespace.
>   /* 
> /home/eax/temp/libpq-failover-8.patch:253: trailing whitespace.
>*  Validate target_server_mode option 
> /home/eax/temp/libpq-failover-8.patch:254: trailing whitespace.
>*/ 
> /home/eax/temp/libpq-failover-8.patch:306: trailing whitespace.
>   appendPQExpBuffer(>errorMessage, 
> error: src/interfaces/libpq/t/001-multihost.pl: already exists in
> working directory
> 
> $ git diff
> ```

Oops. Sorry, my mistake. I forgot to check for untracked files.
Everything is fine.

-- 
Best regards,
Aleksander Alekseev


-- 
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: Implement failover on libpq connect level.

[Aleksander Alekseev]

Hello, Victor.

> I'm sending new version of patch.
> 
> I've replaced readonly option with target_server_type (with JDBC
> compatibility alias targetServerType), 
> 
> use logic of setting defaults based on number of hosts in the connect
> string instead of complicated condition in the state machine,
> 
> added checks for return values of memory allocation function.
> 
> Also, I've solved pg_usleep problem by linking pgsleep.c into libpq
> along with some other libpgport objects which are already linked there.
> 
> Thus client applications don't need to link with libpgport and libpq
> shared library is self-containted.

This patch doesn't apply to master branch:

```
$ git apply ~/temp/libpq-failover-8.patch
/home/eax/temp/libpq-failover-8.patch:184: trailing whitespace.
check: 
/home/eax/temp/libpq-failover-8.patch:252: trailing whitespace.
/* 
/home/eax/temp/libpq-failover-8.patch:253: trailing whitespace.
 *  Validate target_server_mode option 
/home/eax/temp/libpq-failover-8.patch:254: trailing whitespace.
 */ 
/home/eax/temp/libpq-failover-8.patch:306: trailing whitespace.
appendPQExpBuffer(>errorMessage, 
error: src/interfaces/libpq/t/001-multihost.pl: already exists in
working directory

$ git diff
```

-- 
Best regards,
Aleksander Alekseev


-- 
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: Implement failover on libpq connect level.

[Aleksander Alekseev]

> > 8) get_next_element procedure implementation is way too smart (read
> > "complicated"). You could probably just store current list length and
> > generate a random number between 0 and length-1.
> 
> No, algorithm here is more complicated. It must ensure that there would
> not be second attempt to connect to host, for which unsuccessful
> connection attempt was done. So, there is list rearrangement.
> 
> Algorithm for pick random list element by single pass is quite trivial.

Great! In this case I would be _trivial_ for you to write a comment that
describes how this procedure works, what makes you think that it gives a
good distribution in all possible cases (e.g. if there is more than
0x1 elements in a list - why not), etc. Right? :)

-- 
Best regards,
Aleksander Alekseev


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