Re: New feature proposal (trigger)

[Pavel Stehule]

pá 24. 1. 2020 v 8:55 odesílatel Sergiu Velescu 
napsal:

> Hi,
>
>
>
> Yes, please find below few examples.
>
>
>
> OnLogin/Logout.
>
> I want to log/audit each attempt to login (successful and/or not).
>
> Who/how long was logged in DB (who logged in out of business hours (maybe
> deny access)).
>
> Set session variable based on username (or maybe IP address)  - for
> example DATE format.
>
>
>
> OnStartup (or AfterStarted)
>
> I want to start a procedure which check for a specific event in a loop and
> send an email.
>
>
>
> OnDDL
>
> Log every DDL in a DB log table (who/when
> altered/created/dropped/truncated a specific object) and send an email.
>

you can do almost all things today by C extensions or just with Postgres log

Personally I don't thing so doing these things just from Postgres, PL
procedures is good thing

Pavel


> Out of this topic nice to have (I could elaborate any of below topic if
> you are interested in):
>
> Storage quota per user (or schema).
>
> Audit – I know about existence of pgaudit extension but it is far from
> ideal (I compare to Oracle Fine Grained Audit).
>
> Duplicate WAL (to have WAL in 2 different places – for example I take
> backup on separate disk and I want to have a copy of WAL on that disk)
>
> To have something like Oracle SQL Tuning Advisor (for example I have a
> “big” SQL which take longer than it should (probably the optimizer didn’t
> find the pest execution plan in the tame allocated to this) – this tool
> provide the possibility to analyze comprehensive the SQL and offer
> solutions (maybe different execution plan, maybe offer suggestion to create
> a specific index…)).
>
> Best regards.
>
>
>
> *From:* Pavel Stehule 
> *Sent:* Thursday, January 23, 2020 18:39
> *To:* Sergiu Velescu 
> *Cc:* pgsql-hack...@postgresql.org
> *Subject:* Re: New feature proposal (trigger)
>
>
>
>
>
>
>
> čt 23. 1. 2020 v 17:26 odesílatel Sergiu Velescu <
> sergiu.vele...@endava.com> napsal:
>
> Dear PgSQL-Hackers,
>
>
>
> I would like to propose a new feature which is missing in PgSQL but quite
> useful and nice to have (and exists in Oracle and probably in some other
> RDBMS), I speak about “Database Level” triggers: BeforePgStart,
> AfterPgStarted, OnLogin, OnSuccessfulLogin, BeforePGshutdown, OnLogOut – I
> just mentioned some of it but the final events could be different.
>
>
>
> These DB Level triggers are quite useful for example if somebogy want to
> set some PG env. variables depends on user belonging to one or another role
> or want to track who/wen logged in/out, start a stored procedure
> AfterPgStarted and so on.
>
>
>
> Do you have some examples of these useful triggers?
>
>
>
> I don't know any one.
>
>
>
> Regards
>
>
>
> Pavel
>
>
>
>
>
> Thanks!
>
>
> The information in this email is confidential and may be legally
> privileged. It is intended solely for the addressee. Any opinions expressed
> are mine and do not necessarily represent the opinions of the Company.
> Emails are susceptible to interference. If you are not the intended
> recipient, any disclosure, copying, distribution or any action taken or
> omitted to be taken in reliance on it, is strictly prohibited and may be
> unlawful. If you have received this message in error, do not open any
> attachments but please notify the Endava Service Desk on (+44 (0)870 423
> 0187), and delete this message from your system. The sender accepts no
> responsibility for information, errors or omissions in this email, or for
> its use or misuse, or for any act committed or omitted in connection with
> this communication. If in doubt, please verify the authenticity of the
> contents with the sender. Please rely on your own virus checkers as no
> responsibility is taken by the sender for any damage rising out of any bug
> or virus infection.
>
> Endava plc is a company registered in England under company number 5722669
> whose registered office is at 125 Old Broad Street, London, EC2N 1AR,
> United Kingdom. Endava plc is the Endava group holding company and does not
> provide any services to clients. Each of Endava plc and its subsidiaries is
> a separate legal entity and has no liability for another such entity's acts
> or omissions.
>
>

Re: Add pg_file_sync() to adminpack

[Fujii Masao]

On 2020/01/24 16:56, Julien Rouhaud wrote:

On Fri, Jan 24, 2020 at 8:20 AM Fujii Masao  wrote:


On 2020/01/24 15:38, Arthur Zakirov wrote:

On 2020/01/24 14:56, Michael Paquier wrote:

On Fri, Jan 24, 2020 at 01:28:29PM +0900, Arthur Zakirov wrote:

It is compiled and passes the tests. There is the documentation and
it is
built too without an error.

It seems that consensus about the returned type was reached and I
marked the
patch as "Ready for Commiter".


+   fsync_fname_ext(filename, S_ISDIR(fst.st_mode), false, ERROR);
One comment here: should we warn better users in the docs that a fsync
failule will not trigger a PANIC here?  Here, fsync failure on heap
file => ERROR => potential data corruption.


Ah, true. It is possible to add couple sentences that pg_file_sync()
doesn't depend on data_sync_retry GUC and doesn't raise a PANIC even for
database files.


Thanks all for the review!

So, what about the attached patch?
In the patch, I added the following note to the doc.


Note that
 has no effect on this function,
and therefore a PANIC-level error will not be raised even on failure to
flush database files.



We should explicitly mention that this can cause corruption.  How  about:


Note that
 has no effect on this function,
and therefore a PANIC-level error will not be raised even on failure to
flush database files.  If that happens, the underlying database
objects may be corrupted.



IMO that's overkill. If we really need such mention for pg_file_sync(),
we also need to add it for other functions like pg_read_file(),
pg_stat_file(), etc. But, again, which looks overkill.

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

Re: polymorphic table functions light

[Peter Eisentraut]

On 2019-12-16 19:53, Peter Eisentraut wrote:

SQL:2016 has a feature called polymorphic table functions (PTF) that
addresses this.  The full PTF feature is much larger, so I just carved
out this particular piece of functionality.  Here is a link to some
related information:
https://modern-sql.com/blog/2018-11/whats-new-in-oracle-database-18c#ptf

The idea is that you attach a helper function to the main function.  The
helper function is called at parse time with the constant arguments of
the main function call and can compute a result row description (a
TupleDesc in our case).


Here is an updated patch for the record, since the previous patch had 
accumulated some significant merge conflicts.


I will reply to the discussions elsewhere in the thread.

--
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From a05a926ccb3be7f61bda0b075cfa92fe6f7305bf Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Fri, 24 Jan 2020 09:06:38 +0100
Subject: [PATCH v2] Polymorphic table functions

---
 contrib/tablefunc/expected/tablefunc.out  | 46 +++
 contrib/tablefunc/sql/tablefunc.sql   |  8 +++
 contrib/tablefunc/tablefunc--1.0.sql  |  7 +++
 contrib/tablefunc/tablefunc.c | 69 ++
 contrib/xml2/expected/xml2.out| 10 
 contrib/xml2/sql/xml2.sql |  6 ++
 contrib/xml2/xml2--1.1.sql|  6 ++
 contrib/xml2/xpath.c  | 72 +++
 doc/src/sgml/catalogs.sgml| 12 
 doc/src/sgml/queries.sgml |  8 +++
 doc/src/sgml/ref/create_function.sgml | 14 +
 doc/src/sgml/xfunc.sgml   | 66 +
 src/backend/catalog/pg_aggregate.c|  1 +
 src/backend/catalog/pg_proc.c | 11 
 src/backend/commands/functioncmds.c   | 30 +-
 src/backend/commands/proclang.c   |  3 +
 src/backend/commands/typecmds.c   |  1 +
 src/backend/executor/execSRF.c|  1 +
 src/backend/executor/nodeFunctionscan.c   |  1 +
 src/backend/optimizer/prep/prepjointree.c |  1 +
 src/backend/optimizer/util/clauses.c  |  3 +-
 src/backend/parser/gram.y |  7 ++-
 src/backend/parser/parse_relation.c   |  2 +
 src/backend/utils/adt/jsonfuncs.c | 48 +++
 src/backend/utils/fmgr/funcapi.c  | 49 ++-
 src/include/catalog/pg_class.dat  |  2 +-
 src/include/catalog/pg_proc.dat   |  6 +-
 src/include/catalog/pg_proc.h |  4 ++
 src/include/funcapi.h |  1 +
 src/include/parser/kwlist.h   |  1 +
 src/interfaces/ecpg/preproc/ecpg.tokens   |  2 +-
 src/interfaces/ecpg/preproc/ecpg.trailer  | 11 ++--
 src/interfaces/ecpg/preproc/ecpg_kwlist.h |  1 -
 src/test/regress/expected/json.out|  6 ++
 src/test/regress/sql/json.sql |  2 +
 35 files changed, 504 insertions(+), 14 deletions(-)

diff --git a/contrib/tablefunc/expected/tablefunc.out 
b/contrib/tablefunc/expected/tablefunc.out
index fffadc6e1b..485ddfba87 100644
--- a/contrib/tablefunc/expected/tablefunc.out
+++ b/contrib/tablefunc/expected/tablefunc.out
@@ -328,6 +328,29 @@ SELECT * FROM connectby('connectby_text', 'keyid', 
'parent_keyid', 'pos', 'row2'
  row8  | row6 | 3 |   6
 (6 rows)
 
+-- PTF
+SELECT * FROM connectby('connectby_text', 'keyid', 'parent_keyid', 'row2', 0, 
'~');
+ keyid | parent_keyid | level |   branch
+---+--+---+-
+ row2  |  | 0 | row2
+ row4  | row2 | 1 | row2~row4
+ row6  | row4 | 2 | row2~row4~row6
+ row8  | row6 | 3 | row2~row4~row6~row8
+ row5  | row2 | 1 | row2~row5
+ row9  | row5 | 2 | row2~row5~row9
+(6 rows)
+
+SELECT * FROM connectby('connectby_text', 'keyid', 'parent_keyid', 'row2', 0);
+ keyid | parent_keyid | level 
+---+--+---
+ row2  |  | 0
+ row4  | row2 | 1
+ row6  | row4 | 2
+ row8  | row6 | 3
+ row5  | row2 | 1
+ row9  | row5 | 2
+(6 rows)
+
 -- test connectby with int based hierarchy
 CREATE TABLE connectby_int(keyid int, parent_keyid int);
 \copy connectby_int from 'data/connectby_int.data'
@@ -355,6 +378,29 @@ SELECT * FROM connectby('connectby_int', 'keyid', 
'parent_keyid', '2', 0) AS t(k
  9 |5 | 2
 (6 rows)
 
+-- PTF
+SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 0, '~');
+ keyid | parent_keyid | level | branch  
+---+--+---+-
+ 2 |  | 0 | 2
+ 4 |2 | 1 | 2~4
+ 6 |4 | 2 | 2~4~6
+ 8 |6 | 3 | 2~4~6~8
+ 5 |2 | 1 | 2~5
+ 9 |5 | 2 | 2~5~9
+(6 rows)
+
+SELECT * FROM connectby('connectby_int', 'ke

Re: [Proposal] Global temporary tables

[Konstantin Knizhnik]

On 23.01.2020 19:28, 曾文旌(义从) wrote:


I'm trying to improve this part of the implementation in 
global_temporary_table_v7-pg13.patch

Please check my patch and give me feedback.


Thanks

Wenjing




Below is my short review of the patch:

+    /*
+     * For global temp table only
+     * use AccessExclusiveLock for ensure safety
+     */
+    {
+        {
+            "on_commit_delete_rows",
+            "global temp table on commit options",
+            RELOPT_KIND_HEAP | RELOPT_KIND_PARTITIONED,
+            ShareUpdateExclusiveLock
+        },
+        true
+    },


The comment seems to be confusing: it says about AccessExclusiveLock but 
actually uses ShareUpdateExclusiveLock.


- Assert(TransactionIdIsNormal(onerel->rd_rel->relfrozenxid));
-    Assert(MultiXactIdIsValid(onerel->rd_rel->relminmxid));
+    Assert((RELATION_IS_GLOBAL_TEMP(onerel) && 
onerel->rd_rel->relfrozenxid == InvalidTransactionId) ||
+        (!RELATION_IS_GLOBAL_TEMP(onerel) && 
TransactionIdIsNormal(onerel->rd_rel->relfrozenxid)));
+    Assert((RELATION_IS_GLOBAL_TEMP(onerel) && 
onerel->rd_rel->relminmxid == InvalidMultiXactId) ||
+        (!RELATION_IS_GLOBAL_TEMP(onerel) && 
MultiXactIdIsValid(onerel->rd_rel->relminmxid)));


It is actually equivalent to:

Assert(RELATION_IS_GLOBAL_TEMP(onerel) ^ 
TransactionIdIsNormal(onerel->rd_rel->relfrozenxid);
Assert(RELATION_IS_GLOBAL_TEMP(onerel) ^ 
MultiXactIdIsValid(onerel->rd_rel->relminmxid));


+    /* clean temp relation files */
+    if (max_active_gtt > 0)
+        RemovePgTempFiles();
+
 /*

I wonder why do we need some special check for GTT here.
From my point of view cleanup at startup of local storage of temp 
tables should be performed in the same way for local and global temp tables.



-    new_rel_reltup->relfrozenxid = relfrozenxid;
-    new_rel_reltup->relminmxid = relminmxid;
+    /* global temp table not remember transaction info in catalog */
+    if (relpersistence == RELPERSISTENCE_GLOBAL_TEMP)
+    {
+        new_rel_reltup->relfrozenxid = InvalidTransactionId;
+        new_rel_reltup->relminmxid = InvalidMultiXactId;
+    }
+    else
+    {
+        new_rel_reltup->relfrozenxid = relfrozenxid;
+        new_rel_reltup->relminmxid = relminmxid;
+    }
+


Why do we need to do it for GTT?
Did you check that there will be no problems with GTT in case of XID 
wraparound?
Right now if you create temp table and keep session open, then it will 
block XID wraparound.


+    /* We allow to drop global temp table only this session use it */
+    if (RELATION_IS_GLOBAL_TEMP(rel))
+    {
+        if (is_other_backend_use_gtt(rel->rd_node))
+            elog(ERROR, "can not drop relation when other backend 
attached this global temp table");

+    }
+

Here we once again introduce incompatibility with normal (permanent) tables.
Assume that DBA or programmer need to change format of GTT. But there 
are some active sessions which have used this GTT sometime in the past.

We will not be able to drop this GTT until all this sessions are terminated.
I do not think that it is acceptable behaviour.

+        LOCKMODE    lockmode = AccessExclusiveLock;
+
+        /* truncate global temp table only need RowExclusiveLock */
+        if (get_rel_persistence(rid) == RELPERSISTENCE_GLOBAL_TEMP)
+            lockmode = RowExclusiveLock;


What are the reasons of using RowExclusiveLock for GTT instead of 
AccessExclusiveLock?
Yes, GTT data is access only by one backend so no locking here seems to 
be needed at all.
But I wonder what are the motivations/benefits of using weaker lock 
level here?

There should be no conflicts in any case...

+        /* We allow to create index on global temp table only this 
session use it */

+        if (is_other_backend_use_gtt(heapRelation->rd_node))
+            elog(ERROR, "can not create index when have other backend 
attached this global temp table");

+

The same argument as in case of dropping GTT: I do not think that 
prohibiting DLL operations on GTT used by more than one backend is bad idea.


+    /* global temp table not support foreign key constraint yet */
+    if (RELATION_IS_GLOBAL_TEMP(pkrel))
+        ereport(ERROR,
+                (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                 errmsg("referenced relation \"%s\" is not a global 
temp table",

+                        RelationGetRelationName(pkrel;
+

Why do we need to prohibit foreign key constraint on GTT?

+    /*
+     * Global temp table get frozenxid from MyProc
+     * to avoid the vacuum truncate clog that gtt need.
+     */
+    if (max_active_gtt > 0)
+    {
+        TransactionId oldest_gtt_frozenxid =
+            list_all_session_gtt_frozenxids(0, NULL, NULL, NULL);
+
+        if (TransactionIdIsNormal(oldest_gtt_frozenxid) &&
+            TransactionIdPrecedes(oldest_gtt_frozenxid, newFrozenXid))
+        {
+            ereport(WARNING,
+                (errmsg("global temp table oldest FrozenXid is far in 
the past"),
+                 errhint("ple

Re: polymorphic table functions light

[Peter Eisentraut]

On 2019-12-16 22:13, Tom Lane wrote:

Hm.  Given that this involves a function-taking-and-returning-internal,
I think it's fairly silly to claim that it is implementing a SQL-standard
feature, or even a subset or related feature.  Nor do I see a pathway
whereby this might end in a feature you could use without writing C code.



I think we'd be better off to address this by extending the existing
"support function" infrastructure by inventing a new support request type,


I definitely want to make it work in a way that does not require writing 
C code.  My idea was to create a new type, perhaps called "descriptor", 
that represents essentially a tuple descriptor.  (It could be exactly a 
TupleDesc, as this patch does, or something similar.)  For the sake of 
discussion, we could use JSON as the text representation of this.  Then 
a PL/pgSQL function or something else high level could easily be written 
to assemble this.  Interesting use cases are for example in the area of 
using PL/Perl or PL/Python for unpacking some serialization format using 
existing modules in those languages.


The SQL standard has the option of leaving the call signatures of the 
PTF support functions implementation defined, so this approach would 
appear to be within the spirit of the specification.


Obviously, there is a lot of leg work to be done between here and there, 
but it seems doable.  The purpose of this initial patch submission was 
to get some opinions on the basic idea of "determine result tuple 
structure by calling helper function at parse time", and so far no one 
has fallen off their chair from that, so I'm encouraged. ;-)


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

Re: [Proposal] Global temporary tables

[Konstantin Knizhnik]

On 23.01.2020 23:47, Robert Haas wrote:

On Sat, Jan 11, 2020 at 8:51 PM Tomas Vondra
 wrote:

I proposed just ignoring those new indexes because it seems much simpler
than alternative solutions that I can think of, and it's not like those
other solutions don't have other issues.

+1.


For example, I've looked at the "on demand" building as implemented in
global_private_temp-8.patch, I kinda doubt adding a bunch of index build
calls into various places in index code seems somewht suspicious.

+1. I can't imagine that's a safe or sane thing to do.



As far as you know there are two versions of GTT implementations now.
And we are going to merge them into single patch.
But there are some principle question concerning provided functionality 
which has to be be discussed:
should we prohibit DDL on GTT if there are more than one sessions using 
it. It includes creation/dropping indexes, dropping table, altering table...


If the answer is "yes", then the question whether to populate new 
indexes with data is no relevant at all, because such situation will not 
be possible.
But in this case we will get incompatible behavior with normal 
(permanent) tables and it seems to be very inconvenient from DBA point 
of view:
it will be necessary to enforce all clients to close their sessions to 
perform some DDL manipulations with GTT.
Some DDLs will be very difficult to implement if GTT is used by more 
than one backend, for example altering table schema.


My current solution is to allow creation/droping index on GTT and 
dropping table itself, while prohibit alter schema at all for GTT.
Wenjing's approach is to prohibit any DDL if GTT is used by more than 
one backend.


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

Re: polymorphic table functions light

[Peter Eisentraut]

On 2019-12-20 01:30, Vik Fearing wrote:

On 16/12/2019 22:13, Tom Lane wrote:

That being the case, I'm not in favor of using up SQL syntax space for it
if we don't have to.


Do I understand correctly that you are advocating *against* using
standard SQL syntax for a feature that is defined by the SQL Standard
and that we have no similar implementation for?


On the question of using SQL syntax or not for this, there are a couple 
of arguments I'm considering.


First, the SQL standard explicitly permits not implementing the exact 
signatures of the PTF component procedures; see feature code B208. 
While this does not literally permit diverging on the CREATE FUNCTION 
syntax, it's clear that they expect that the creation side of this will 
have some incompatibilities.  The existing practices of other vendors 
support this observation.  What's more interesting in practice is making 
the invocation side compatible.


Second, set-returning functions in PostgreSQL already exist and in my 
mind it would make sense to make this feature work with existing 
functions or allow easy "upgrades" rather than introducing another 
completely new syntax to do something very similar to what already 
exists.  This wouldn't be a good user experience.  And the full standard 
syntax is also complicated and different enough that it wouldn't be 
trivial to add.


But I'm open to other ideas.

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

Re: Run-time pruning for ModifyTable

[Amit Langote]

On Thu, Jan 23, 2020 at 4:31 PM Amit Langote  wrote:
> Now, the chances of such a big overhaul of how UPDATEs of inheritance
> trees are handled getting into PG 13 seem pretty thin even if I post
> the patch in few days, so perhaps it would make sense to get this
> patch in so that we can give users run-time pruning for UPDATE/DELETE
> in PG 13, provided the code is not very invasive.  If and when the
> aforesaid overhaul takes place, that code would go away along with a
> lot of other code.

Fwiw, I updated the patch, mainly expected/partition_prune.out.  Some
tests in it were failing as a fallout of commits d52eaa09 (pointed out
by Thomas upthread) and 6ef77cf46e8, which are not really related to
the code being changed by the patch.

On the patch itself, it seems straightforward enough.  It simply takes
the feature we have for Append and MergeAppend nodes and adopts it for
ModifyTable which for the purposes of run-time pruning looks very much
like the aforementioned nodes.

Part of the optimizer patch that looks a bit complex is the changes to
inheritance_planner() which is to be expected, because that function
is a complex beast itself.  I have suggestions to modify some comments
around the code added/modified by the patch for clarity; attaching a
delta patch for that.

The executor patch looks pretty benign too.  Diffs that looked a bit
suspicious at first are due to replacing
ModifyTableState.resultRelInfo that is a pointer into
EState.es_result_relations array by an array of ResultRelInfo
pointers, but doing that seems to make the relevant code easier to
follow, especially if you consider the changes that the patch makes to
that code.

I'll set the CF entry to Needs Review, because AFAICS there are no
unaddressed comments.

Thanks,
Amit
diff --git a/src/backend/optimizer/plan/planner.c 
b/src/backend/optimizer/plan/planner.c
index 30d15291e3..c4244e6d29 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -1802,7 +1802,9 @@ inheritance_planner(PlannerInfo *root)
 * When performing UPDATE/DELETE on a partitioned table, if the query 
has
 * a WHERE clause which supports it, we may be able to perform run-time
 * partition pruning.  The following code sets things up to allow this 
to
-* be possible.
+* be possible using the information from partition_root that was used
+* during planning of the SELECT version of this query which we 
performed
+* above.
 */
if (partition_root && !dummy_update)
{
@@ -1810,36 +1812,29 @@ inheritance_planner(PlannerInfo *root)
int i;
 
/*
-* Fetch the target partitioned table from the SELECT version of
-* the query which we performed above.  This may have the base 
quals
+* Fetch the target partitioned table which may have the base 
quals
 * which could allow the run-time pruning to work.
 */
parent_rel = 
partition_root->simple_rel_array[top_parentRTindex];
-
final_rel->baserestrictinfo = parent_rel->baserestrictinfo;
 
-   /* build a list of partitioned rels */
+   /* Collect all non-leaf tables in the partition tree being 
updated. */
i = -1;
while ((i = bms_next_member(parent_relids, i)) > 0)
partitioned_rels = lappend_int(partitioned_rels, i);
 
-
/*
-* In order to build the run-time pruning data we'll need 
append rels
-* any sub-partitioned tables.  If there are some of those and 
the
-* append_rel_array is not already allocated, then do that now.
+* There can only be a single partition tree, the one whose 
root is
+* the query's main target table.
 */
-   if (list_length(partitioned_rels) > 1 &&
-   root->append_rel_array == NULL)
-   root->append_rel_array = palloc0(sizeof(AppendRelInfo 
*) *
-   
 root->simple_rel_array_size);
+   partitioned_rels = list_make1(partitioned_rels);
 
/*
-* There can only be a single partition hierarchy, so it's fine 
to
-* just make a single element list of the partitioned_rels.
+* Update simple_rel_array and append_rel_array so that runtime
+* pruning setup logic can find the relavant partitioned 
relations.
+* Just use the one that the planning of SELECT version of the 
query
+* would have created.
 */
-   partitioned_rels = list_make1(partitioned_rels);
-
i = -1;
while ((i = bms_next_member(parent_relids, i)) >= 0)
{
@@ -1847,11 +18

RE: New feature proposal (trigger)

[Sergiu Velescu]

Hi,

Yes, please find below few examples.

OnLogin/Logout.
I want to log/audit each attempt to login (successful and/or not).
Who/how long was logged in DB (who logged in out of business hours (maybe deny 
access)).
Set session variable based on username (or maybe IP address)  - for example 
DATE format.

OnStartup (or AfterStarted)
I want to start a procedure which check for a specific event in a loop and send 
an email.

OnDDL
Log every DDL in a DB log table (who/when altered/created/dropped/truncated a 
specific object) and send an email.

Out of this topic nice to have (I could elaborate any of below topic if you are 
interested in):
Storage quota per user (or schema).
Audit – I know about existence of pgaudit extension but it is far from ideal (I 
compare to Oracle Fine Grained Audit).
Duplicate WAL (to have WAL in 2 different places – for example I take backup on 
separate disk and I want to have a copy of WAL on that disk)
To have something like Oracle SQL Tuning Advisor (for example I have a “big” 
SQL which take longer than it should (probably the optimizer didn’t find the 
pest execution plan in the tame allocated to this) – this tool provide the 
possibility to analyze comprehensive the SQL and offer solutions (maybe 
different execution plan, maybe offer suggestion to create a specific index…)).
Best regards.

From: Pavel Stehule 
Sent: Thursday, January 23, 2020 18:39
To: Sergiu Velescu 
Cc: pgsql-hack...@postgresql.org
Subject: Re: New feature proposal (trigger)



čt 23. 1. 2020 v 17:26 odesílatel Sergiu Velescu 
mailto:sergiu.vele...@endava.com>> napsal:
Dear PgSQL-Hackers,

I would like to propose a new feature which is missing in PgSQL but quite 
useful and nice to have (and exists in Oracle and probably in some other 
RDBMS), I speak about “Database Level” triggers: BeforePgStart, AfterPgStarted, 
OnLogin, OnSuccessfulLogin, BeforePGshutdown, OnLogOut – I just mentioned some 
of it but the final events could be different.

These DB Level triggers are quite useful for example if somebogy want to set 
some PG env. variables depends on user belonging to one or another role or want 
to track who/wen logged in/out, start a stored procedure AfterPgStarted and so 
on.

Do you have some examples of these useful triggers?

I don't know any one.

Regards

Pavel


Thanks!

The information in this email is confidential and may be legally privileged. It 
is intended solely for the addressee. Any opinions expressed are mine and do 
not necessarily represent the opinions of the Company. Emails are susceptible 
to interference. If you are not the intended recipient, any disclosure, 
copying, distribution or any action taken or omitted to be taken in reliance on 
it, is strictly prohibited and may be unlawful. If you have received this 
message in error, do not open any attachments but please notify the Endava 
Service Desk on (+44 (0)870 423 0187), and delete this message from your 
system. The sender accepts no responsibility for information, errors or 
omissions in this email, or for its use or misuse, or for any act committed or 
omitted in connection with this communication. If in doubt, please verify the 
authenticity of the contents with the sender. Please rely on your own virus 
checkers as no responsibility is taken by the sender for any damage rising out 
of any bug or virus infection.

Endava plc is a company registered in England under company number 5722669 
whose registered office is at 125 Old Broad Street, London, EC2N 1AR, United 
Kingdom. Endava plc is the Endava group holding company and does not provide 
any services to clients. Each of Endava plc and its subsidiaries is a separate 
legal entity and has no liability for another such entity's acts or omissions.

RE: New feature proposal (trigger)

[Sergiu Velescu]

Hi,

Could you please elaborate – what do you mean by “…you can do almost all things 
today by C extensions…” – does these extensions already exists or I have to 
develop it?
If these extensions exists and developed by somebody else (not in PG core) then 
nobody will install it where sensitive information exists (at least you will 
not be able to pass the PCI-DSS audit).
If I have to develop it – then I have 2 option 1) to develop it or 2) to use 
other RDBMS which already have this implemented.

For enterprise class solutions it is vital to have the possibility to keep 
track of actions in DB (who/when logged-in/out, which statement run and so on), 
this is even more important than performance because if I need more performance 
I probably could increase the hardware procession power (CPU/RAM/IOPS) but if I 
have no audit I have no choice…

I know PostgreSQL is free solution and I can’t expect it to have everything a 
commercial RDBMS have but at least we should start to think to implement this!

Have a nice day!

From: Pavel Stehule 
Sent: Friday, January 24, 2020 10:03
To: Sergiu Velescu 
Cc: pgsql-hack...@postgresql.org
Subject: Re: New feature proposal (trigger)



pá 24. 1. 2020 v 8:55 odesílatel Sergiu Velescu 
mailto:sergiu.vele...@endava.com>> napsal:
Hi,

Yes, please find below few examples.

OnLogin/Logout.
I want to log/audit each attempt to login (successful and/or not).
Who/how long was logged in DB (who logged in out of business hours (maybe deny 
access)).
Set session variable based on username (or maybe IP address)  - for example 
DATE format.

OnStartup (or AfterStarted)
I want to start a procedure which check for a specific event in a loop and send 
an email.

OnDDL
Log every DDL in a DB log table (who/when altered/created/dropped/truncated a 
specific object) and send an email.

you can do almost all things today by C extensions or just with Postgres log

Personally I don't thing so doing these things just from Postgres, PL 
procedures is good thing

Pavel


Out of this topic nice to have (I could elaborate any of below topic if you are 
interested in):
Storage quota per user (or schema).
Audit – I know about existence of pgaudit extension but it is far from ideal (I 
compare to Oracle Fine Grained Audit).
Duplicate WAL (to have WAL in 2 different places – for example I take backup on 
separate disk and I want to have a copy of WAL on that disk)
To have something like Oracle SQL Tuning Advisor (for example I have a “big” 
SQL which take longer than it should (probably the optimizer didn’t find the 
pest execution plan in the tame allocated to this) – this tool provide the 
possibility to analyze comprehensive the SQL and offer solutions (maybe 
different execution plan, maybe offer suggestion to create a specific index…)).
Best regards.

From: Pavel Stehule mailto:pavel.steh...@gmail.com>>
Sent: Thursday, January 23, 2020 18:39
To: Sergiu Velescu mailto:sergiu.vele...@endava.com>>
Cc: pgsql-hack...@postgresql.org
Subject: Re: New feature proposal (trigger)



čt 23. 1. 2020 v 17:26 odesílatel Sergiu Velescu 
mailto:sergiu.vele...@endava.com>> napsal:
Dear PgSQL-Hackers,

I would like to propose a new feature which is missing in PgSQL but quite 
useful and nice to have (and exists in Oracle and probably in some other 
RDBMS), I speak about “Database Level” triggers: BeforePgStart, AfterPgStarted, 
OnLogin, OnSuccessfulLogin, BeforePGshutdown, OnLogOut – I just mentioned some 
of it but the final events could be different.

These DB Level triggers are quite useful for example if somebogy want to set 
some PG env. variables depends on user belonging to one or another role or want 
to track who/wen logged in/out, start a stored procedure AfterPgStarted and so 
on.

Do you have some examples of these useful triggers?

I don't know any one.

Regards

Pavel


Thanks!

The information in this email is confidential and may be legally privileged. It 
is intended solely for the addressee. Any opinions expressed are mine and do 
not necessarily represent the opinions of the Company. Emails are susceptible 
to interference. If you are not the intended recipient, any disclosure, 
copying, distribution or any action taken or omitted to be taken in reliance on 
it, is strictly prohibited and may be unlawful. If you have received this 
message in error, do not open any attachments but please notify the Endava 
Service Desk on (+44 (0)870 423 0187), and delete this message from your 
system. The sender accepts no responsibility for information, errors or 
omissions in this email, or for its use or misuse, or for any act committed or 
omitted in connection with this communication. If in doubt, please verify the 
authenticity of the contents with the sender. Please rely on your own virus 
checkers as no responsibility is taken by the sender for any damage rising out 
of any bug or virus infection.

Endava plc is a company registered in England under company number 57226

[PoC] Non-volatile WAL buffer

[Takashi Menjo]

Dear hackers,

I propose "non-volatile WAL buffer," a proof-of-concept new feature.  It
enables WAL records to be durable without output to WAL segment files by
residing on persistent memory (PMEM) instead of DRAM.  It improves database
performance by reducing copies of WAL and shortening the time of write
transactions.

I attach the first patchset that can be applied to PostgreSQL 12.0 (refs/
tags/REL_12_0).  Please see README.nvwal (added by the patch 0003) to use
the new feature.

PMEM [1] is fast, non-volatile, and byte-addressable memory installed into
DIMM slots. Such products have been already available.  For example, an
NVDIMM-N is a type of PMEM module that contains both DRAM and NAND flash.
It can be accessed like a regular DRAM, but on power loss, it can save its
contents into flash area.  On power restore, it performs the reverse, that
is, the contents are copied back into DRAM.  PMEM also has been already
supported by major operating systems such as Linux and Windows, and new
open-source libraries such as Persistent Memory Development Kit (PMDK) [2].
Furthermore, several DBMSes have started to support PMEM.

It's time for PostgreSQL.  PMEM is faster than a solid state disk and
naively can be used as a block storage.  However, we cannot gain much
performance in that way because it is so fast that the overhead of
traditional software stacks now becomes unignorable, such as user buffers,
filesystems, and block layers.  Non-volatile WAL buffer is a work to make
PostgreSQL PMEM-aware, that is, accessing directly to PMEM as a RAM to
bypass such overhead and achieve the maximum possible benefit.  I believe
WAL is one of the most important modules to be redesigned for PMEM because
it has assumed slow disks such as HDDs and SSDs but PMEM is not so.

This work is inspired by "Non-volatile Memory Logging" talked in PGCon
2016 [3] to gain more benefit from PMEM than my and Yoshimi's previous
work did [4][5].  I submitted a talk proposal for PGCon in this year, and
have measured and analyzed performance of my PostgreSQL with non-volatile
WAL buffer, comparing with the original one that uses PMEM as "a faster-
than-SSD storage."  I will talk about the results if accepted.

Best regards,
Takashi Menjo

[1] Persistent Memory (SNIA)
  https://www.snia.org/PM
[2] Persistent Memory Development Kit (pmem.io)
  https://pmem.io/pmdk/ 
[3] Non-volatile Memory Logging (PGCon 2016)
  https://www.pgcon.org/2016/schedule/track/Performance/945.en.html
[4] Introducing PMDK into PostgreSQL (PGCon 2018)
  https://www.pgcon.org/2018/schedule/events/1154.en.html
[5] Applying PMDK to WAL operations for persistent memory (pgsql-hackers)
  
https://www.postgresql.org/message-id/c20d38e97bcb33dad59e...@lab.ntt.co.jp

-- 
Takashi Menjo 
NTT Software Innovation Center




0001-Support-GUCs-for-external-WAL-buffer.patch
Description: Binary data


0002-Non-volatile-WAL-buffer.patch
Description: Binary data


0003-README-for-non-volatile-WAL-buffer.patch
Description: Binary data

Re: [Proposal] Global temporary tables

[Pavel Stehule]

pá 24. 1. 2020 v 9:39 odesílatel Konstantin Knizhnik <
k.knizh...@postgrespro.ru> napsal:

>
>
> On 23.01.2020 23:47, Robert Haas wrote:
> > On Sat, Jan 11, 2020 at 8:51 PM Tomas Vondra
> >  wrote:
> >> I proposed just ignoring those new indexes because it seems much simpler
> >> than alternative solutions that I can think of, and it's not like those
> >> other solutions don't have other issues.
> > +1.
> >
> >> For example, I've looked at the "on demand" building as implemented in
> >> global_private_temp-8.patch, I kinda doubt adding a bunch of index build
> >> calls into various places in index code seems somewht suspicious.
> > +1. I can't imagine that's a safe or sane thing to do.
> >
>
> As far as you know there are two versions of GTT implementations now.
> And we are going to merge them into single patch.
> But there are some principle question concerning provided functionality
> which has to be be discussed:
> should we prohibit DDL on GTT if there are more than one sessions using
> it. It includes creation/dropping indexes, dropping table, altering
> table...
>
> If the answer is "yes", then the question whether to populate new
> indexes with data is no relevant at all, because such situation will not
> be possible.
> But in this case we will get incompatible behavior with normal
> (permanent) tables and it seems to be very inconvenient from DBA point
> of view:
> it will be necessary to enforce all clients to close their sessions to
> perform some DDL manipulations with GTT.
> Some DDLs will be very difficult to implement if GTT is used by more
> than one backend, for example altering table schema.
>
> My current solution is to allow creation/droping index on GTT and
> dropping table itself, while prohibit alter schema at all for GTT.
> Wenjing's approach is to prohibit any DDL if GTT is used by more than
> one backend.
>

When I create index on GTT in one session, then I don't expect creating
same index in all other sessions that uses same GTT.

But I can imagine to creating index on GTT enforces index in current
session, and for other sessions this index will be invalid to end of
session.

Regards

Pavel

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

Re: New feature proposal (trigger)

[Pavel Stehule]

pá 24. 1. 2020 v 10:08 odesílatel Sergiu Velescu 
napsal:

> Hi,
>
>
>
> Could you please elaborate – what do you mean by “…you can do almost all
> things today by C extensions…” – does these extensions already exists or I
> have to develop it?
>
> If these extensions exists and developed by somebody else (not in PG core)
> then nobody will install it where sensitive information exists (at least
> you will not be able to pass the PCI-DSS audit).
>
> If I have to develop it – then I have 2 option 1) to develop it or 2) to
> use other RDBMS which already have this implemented.
>
>
>
> For enterprise class solutions it is vital to have the possibility to keep
> track of actions in DB (who/when logged-in/out, which statement run and so
> on), this is even more important than performance because if I need more
> performance I probably could increase the hardware procession power
> (CPU/RAM/IOPS) but if I have no audit I have no choice…
>
>
>
> I know PostgreSQL is free solution and I can’t expect it to have
> everything a commercial RDBMS have but at least we should start to think to
> implement this!
>

lot of this does pg_audit https://www.pgaudit.org/

these is a possibility to log - loging/logout, using DDL. - you can process
postgresql log.

regards

Pavel



>
> Have a nice day!
>
>
>
> *From:* Pavel Stehule 
> *Sent:* Friday, January 24, 2020 10:03
> *To:* Sergiu Velescu 
> *Cc:* pgsql-hack...@postgresql.org
> *Subject:* Re: New feature proposal (trigger)
>
>
>
>
>
>
>
> pá 24. 1. 2020 v 8:55 odesílatel Sergiu Velescu 
> napsal:
>
> Hi,
>
>
>
> Yes, please find below few examples.
>
>
>
> OnLogin/Logout.
>
> I want to log/audit each attempt to login (successful and/or not).
>
> Who/how long was logged in DB (who logged in out of business hours (maybe
> deny access)).
>
> Set session variable based on username (or maybe IP address)  - for
> example DATE format.
>
>
>
> OnStartup (or AfterStarted)
>
> I want to start a procedure which check for a specific event in a loop and
> send an email.
>
>
>
> OnDDL
>
> Log every DDL in a DB log table (who/when
> altered/created/dropped/truncated a specific object) and send an email.
>
>
>
> you can do almost all things today by C extensions or just with Postgres
> log
>
>
>
> Personally I don't thing so doing these things just from Postgres, PL
> procedures is good thing
>
>
>
> Pavel
>
>
>
>
>
> Out of this topic nice to have (I could elaborate any of below topic if
> you are interested in):
>
> Storage quota per user (or schema).
>
> Audit – I know about existence of pgaudit extension but it is far from
> ideal (I compare to Oracle Fine Grained Audit).
>
> Duplicate WAL (to have WAL in 2 different places – for example I take
> backup on separate disk and I want to have a copy of WAL on that disk)
>
> To have something like Oracle SQL Tuning Advisor (for example I have a
> “big” SQL which take longer than it should (probably the optimizer didn’t
> find the pest execution plan in the tame allocated to this) – this tool
> provide the possibility to analyze comprehensive the SQL and offer
> solutions (maybe different execution plan, maybe offer suggestion to create
> a specific index…)).
>
> Best regards.
>
>
>
> *From:* Pavel Stehule 
> *Sent:* Thursday, January 23, 2020 18:39
> *To:* Sergiu Velescu 
> *Cc:* pgsql-hack...@postgresql.org
> *Subject:* Re: New feature proposal (trigger)
>
>
>
>
>
>
>
> čt 23. 1. 2020 v 17:26 odesílatel Sergiu Velescu <
> sergiu.vele...@endava.com> napsal:
>
> Dear PgSQL-Hackers,
>
>
>
> I would like to propose a new feature which is missing in PgSQL but quite
> useful and nice to have (and exists in Oracle and probably in some other
> RDBMS), I speak about “Database Level” triggers: BeforePgStart,
> AfterPgStarted, OnLogin, OnSuccessfulLogin, BeforePGshutdown, OnLogOut – I
> just mentioned some of it but the final events could be different.
>
>
>
> These DB Level triggers are quite useful for example if somebogy want to
> set some PG env. variables depends on user belonging to one or another role
> or want to track who/wen logged in/out, start a stored procedure
> AfterPgStarted and so on.
>
>
>
> Do you have some examples of these useful triggers?
>
>
>
> I don't know any one.
>
>
>
> Regards
>
>
>
> Pavel
>
>
>
>
>
> Thanks!
>
>
> The information in this email is confidential and may be legally
> privileged. It is intended solely for the addressee. Any opinions expressed
> are mine and do not necessarily represent the opinions of the Company.
> Emails are susceptible to interference. If you are not the intended
> recipient, any disclosure, copying, distribution or any action taken or
> omitted to be taken in reliance on it, is strictly prohibited and may be
> unlawful. If you have received this message in error, do not open any
> attachments but please notify the Endava Service Desk on (+44 (0)870 423
> 0187), and delete this message from your system. The sender accepts no
> responsibility for information, errors or omissions in this 

Re: BUG #16171: Potential malformed JSON in explain output

[Hamid Akhtar]

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've reviewed and verified this patch and IMHO, this is ready to be committed.


So I have verified this patch against the tip of REL_12_STABLE branch (commit 
c4c76d19).
- git apply  works without issues.

Although the patch description mentions that it fixes a malformed JSON in 
explain output by creating a "Plan" group, this also fixes the same 
malformation issue for XML and YAML formats. It does not impact the text output 
though.

I'm sharing the problematic part of output for an unpatched version for JSON, 
XML, and YAML formats:
-- JSON
   "Plans": [
 "Subplans Removed": 1,
 {
   "Node Type": "Seq Scan",

-- XML
   
 1
 

-- YAML
 Plans:
   Subplans Removed: 1
   - Node Type: "Seq Scan"

The patched version gives the following and correct output:
-- JSON
   "Plans": [
 {
   "Subplans Removed": 1
 }, 
 {
   "Node Type": "Seq Scan",

-- XML
   
 
   1
 
 

-- YAML
 Plans:
   - Subplans Removed: 1
   - Node Type: "Seq Scan"

Following is the query that I used for validating the output. I picked it up 
(and simplified) from "src/test/regress/sql/partition_prune.sql". You can 
probably come up with a simpler query, but this does the job. The query below 
gives the output in JSON format:

create table ab (a int not null, b int not null) partition by list (a);
create table ab_a2 partition of ab for values in(2) partition by list (b);
create table ab_a2_b1 partition of ab_a2 for values in (1);
create table ab_a1 partition of ab for values in(1) partition by list (b);
create table ab_a1_b1 partition of ab_a1 for values in (2);

-- Disallow index only scans as concurrent transactions may stop visibility
-- bits being set causing "Heap Fetches" to be unstable in the EXPLAIN ANALYZE
-- output.
set enable_indexonlyscan = off;

prepare ab_q1 (int, int, int) as
select * from ab where a between $1 and $2 and b <= $3;

-- Execute query 5 times to allow choose_custom_plan
-- to start considering a generic plan.
execute ab_q1 (1, 8, 3);
execute ab_q1 (1, 8, 3);
execute ab_q1 (1, 8, 3);
execute ab_q1 (1, 8, 3);
execute ab_q1 (1, 8, 3);

explain (format json, analyze, costs off, summary off, timing off) execute 
ab_q1 (2, 2, 3);

deallocate ab_q1;
drop table ab;


The new status of this patch is: Ready for Committer

Re: New feature proposal (trigger)

[Christoph Moench-Tegeder]

## Sergiu Velescu (sergiu.vele...@endava.com):

> OnLogin/Logout.
> I want to log/audit each attempt to login (successful and/or not).

log_connections/log_disconnections

> Who/how long was logged in DB (who logged in out of business hours
> (maybe deny access)).

Use PAM authentication.

> Set session variable based on username (or maybe IP address)  -
> for example DATE format.

"Based on user name": ALTER ROLE

> OnStartup (or AfterStarted)
> I want to start a procedure which check for a specific event in a loop
> and send an email.

That sounds like "problematic architecture" right from the start:
- sending emails in a database transaction is not a good idea
- active-waiting for events ("in a loop") is inefficient, try writing
  to a queue table and have a daemon read from that.

> OnDDL
> Log every DDL in a DB log table (who/when altered/created/dropped/
> truncated a specific object) and send an email.

Event Triggers
https://www.postgresql.org/docs/current/event-triggers.html

> Duplicate WAL (to have WAL in 2 different places – for example I take
> backup on separate disk and I want to have a copy of WAL on that disk)

We have streaming replication/pg_receivewal or file based archiving,
both also wrapped in practical products like barman, pgbackrest, ...

Regards,
Christoph

-- 
Spare Space

Re: [Proposal] Global temporary tables

[Konstantin Knizhnik]

On 24.01.2020 12:09, Pavel Stehule wrote:



pá 24. 1. 2020 v 9:39 odesílatel Konstantin Knizhnik 
mailto:k.knizh...@postgrespro.ru>> napsal:




On 23.01.2020 23:47, Robert Haas wrote:
> On Sat, Jan 11, 2020 at 8:51 PM Tomas Vondra
> mailto:tomas.von...@2ndquadrant.com>> wrote:
>> I proposed just ignoring those new indexes because it seems
much simpler
>> than alternative solutions that I can think of, and it's not
like those
>> other solutions don't have other issues.
> +1.
>
>> For example, I've looked at the "on demand" building as
implemented in
>> global_private_temp-8.patch, I kinda doubt adding a bunch of
index build
>> calls into various places in index code seems somewht suspicious.
> +1. I can't imagine that's a safe or sane thing to do.
>

As far as you know there are two versions of GTT implementations now.
And we are going to merge them into single patch.
But there are some principle question concerning provided
functionality
which has to be be discussed:
should we prohibit DDL on GTT if there are more than one sessions
using
it. It includes creation/dropping indexes, dropping table,
altering table...

If the answer is "yes", then the question whether to populate new
indexes with data is no relevant at all, because such situation
will not
be possible.
But in this case we will get incompatible behavior with normal
(permanent) tables and it seems to be very inconvenient from DBA
point
of view:
it will be necessary to enforce all clients to close their
sessions to
perform some DDL manipulations with GTT.
Some DDLs will be very difficult to implement if GTT is used by more
than one backend, for example altering table schema.

My current solution is to allow creation/droping index on GTT and
dropping table itself, while prohibit alter schema at all for GTT.
Wenjing's approach is to prohibit any DDL if GTT is used by more than
one backend.


When I create index on GTT in one session, then I don't expect 
creating same index in all other sessions that uses same GTT.


But I can imagine to creating index on GTT enforces index in current 
session, and for other sessions this index will be invalid to end of 
session.


So there are three possible alternatives:

1. Prohibit index creation of GTT when it used by more than once session.
2. Create index and populate them with data in all sessions using this GTT.
3. Create index only in current session and do not allow to use it in 
all other sessions already using this GTT (but definitely allow to use 
it in new sessions).


1 is Wenjing's approach, 2 - is my approach, 3 - is your suggestion :)

I can construct the following table with pro/cons of each approach:

Approach
Compatibility with normal table
User (DBA) friendly
Complexity of implementation
Consistency
1
-
1: requires restart of all sessions to perform operation
2: requires global cache of GTT
3/: /no man, no problem
2
+
	3: if index is created then it is actually needed, isn't it? 	1: use 
existed functionality to create index

2: if alter schema is prohibited
3
-
2: requires restart of all sessions to use created index
	3: requires some mechanism for prohibiting index created after first 
session access to GTT

1: can perform DDL but do no see effect of it




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

Re: Allow to_date() and to_timestamp() to accept localized names

[Juan José Santamaría Flecha]

On Thu, Jan 23, 2020 at 11:00 PM Tom Lane  wrote:

Thank you for your time looking into this.

Here's a v7 patch, rebased over my recent hacking, and addressing
> most of the complaints I raised in <31691.1579648...@sss.pgh.pa.us>.
> However, I've got some new complaints now that I've looked harder
> at the code:
>
> * It's not exactly apparent to me why this code should be concerned
> about non-normalized characters when noplace else in the backend is.
> I think we should either rip that out entirely, or move the logic
> into str_tolower (and hence also str_toupper etc).  I'd tend to favor
> the former, but of course I don't speak any languages where this would
> be an issue.  Perhaps a better idea is to invent a new SQL function
> that normalizes a given string, and expect users to call that first
> if they'd like to_date() to match unnormalized text.
>
>
There is an open patch that will make the normalization functionality user
visible [1]. So, if a user can call to_date(normalize('01 ŞUB 2010'), 'DD
TMMON ') I would vote to drop the normalization logic inside this patch
altogether.

* I have no faith in this calculation that decides how long the match
> length was:
>
> *len = element_len + name_len - norm_len;
>
> I seriously doubt that this does the right thing if either the
> normalization or the downcasing changed the byte length of the
> string.  I'm not actually sure how we can do that correctly.
> There's no good way to know whether the changes happened within
> the part of the "name" string that we matched, or the part beyond
> what we matched, but we only want to count the former.  So this
> seems like a pretty hard problem, and even if this logic is somehow
> correct as it stands, it needs a comment explaining why.
>
>
The proper logic would come from do_to_timestamp() receiving a normalized
"date_txt" input, so we do not operate with unnormalize and normalize
strings at the same time.


> * I'm still concerned about the risk of integer overflow in the
> string allocations in seq_search_localized().  Those need to be
> adjusted to be more paranoid, similar to the code in e.g. str_tolower.
>

Please find attached a patch with the normalization logic removed, thus no
direct allocations in seq_search_localized().

I would like to rise a couple of questions myself:

* When compiled with DEBUG_TO_FROM_CHAR, there is a warning "‘dump_node’
defined but not used". Should we drop this function or uncomment its usage?

* Would it be worth moving str_tolower(localized_name)
from seq_search_localized() into cache_locale_time()?

[1]
https://www.postgresql.org/message-id/014866c8-d7ff-2a4f-c185-cf7e3ceb7028%402ndquadrant.com

Regards,

Juan José Santamaría Flecha


0001-Allow-localized-month-names-to_date-v8.patch
Description: Binary data

Re: Preserve versions of initdb-created collations in pg_upgrade

[Peter Eisentraut]

On 2019-12-21 09:01, Thomas Munro wrote:

I think this problem goes away if we commit the per-object collation
version patch set[1].  It drops the collversion column, and Julien's
recent versions handle pg_upgrade quite well, as long as a collation
by the same name exists in the target cluster.  In that universe, if
initdb didn't create them, we'd have to tell people to create all
necessary collations manually before doing a pg_upgrade into it, and
that doesn't seem great.  Admittedly there might be some weird cases
where a collation is somehow completely different but has the same
name.


Setting this patch to Returned with Feedback.

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

Re: Busted(?) optimization in ATAddForeignKeyConstraint

[Peter Eisentraut]

On 2020-01-23 23:11, Tom Lane wrote:

I happened to notice this comment in the logic in
ATAddForeignKeyConstraint that tries to decide if it can skip
revalidating a foreign-key constraint after a DDL change:

  * Since we require that all collations share the same notion of
  * equality (which they do, because texteq reduces to bitwise
  * equality), we don't compare collation here.

Hasn't this been broken by the introduction of nondeterministic
collations?


I'm not very familiar with the logic in this function, but I think this 
might be okay because the foreign-key equality comparisons are done with 
the collation of the primary key, which doesn't change here AFAICT.


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

Re: Busted(?) optimization in ATAddForeignKeyConstraint

[Peter Eisentraut]

On 2020-01-24 01:21, Thomas Munro wrote:

On Fri, Jan 24, 2020 at 11:12 AM Tom Lane  wrote:

I happened to notice this comment in the logic in
ATAddForeignKeyConstraint that tries to decide if it can skip
revalidating a foreign-key constraint after a DDL change:

  * Since we require that all collations share the same notion of
  * equality (which they do, because texteq reduces to bitwise
  * equality), we don't compare collation here.

Hasn't this been broken by the introduction of nondeterministic
collations?


Similar words appear in the comment for ri_GenerateQualCollation().


The calls to this function are all conditional on 
!get_collation_isdeterministic().  The comment should perhaps be changed.


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

Re: [PoC] Non-volatile WAL buffer

[Fabien COELHO]

Hello,

+1 on the idea.

By quickly looking at the patch, I notice that there are no tests.

Is it possible to emulate somthing without the actual hardware, at least 
for testing purposes?


--
Fabien.

Re: Setting min/max TLS protocol in clientside libpq

[Daniel Gustafsson]

> On 17 Jan 2020, at 03:38, Michael Paquier  wrote:
> 
> On Fri, Jan 17, 2020 at 10:09:54AM +0900, Michael Paquier wrote:
>> Could you please rebase and fix the remaining pieces of the patch?
> 
> And while I remember, you may want to add checks for incorrect bounds
> when validating the values in fe-connect.c...  The same arguments as
> for the backend part apply because we'd want to make the
> implementation a maximum pluggable with all SSL libraries.

Agreed.

Attached is a v5 of the patch which hopefully address all the comments raised,
sorry for the delay.

cheers ./daniel



libpq_minmaxproto_v5.patch
Description: Binary data

Re: Add support for automatically updating Unicode derived files

[Peter Eisentraut]

On 2020-01-20 16:43, Tom Lane wrote:

Peter Eisentraut  writes:

On 2020-01-15 01:37, Tom Lane wrote:

This patch is making src/tools/pginclude/headerscheck unhappy:
./src/include/common/unicode_combining_table.h:3: error: array type has 
incomplete element type



Hmm, this file is only meant to be included inside one particular
function.  Making it standalone includable would seem to be unnecessary.
   What should we do?


Well, we could make it a documented exception in headerscheck and
cpluspluscheck.


OK, done.

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

Re: [HACKERS] Block level parallel vacuum

[Mahendra Singh Thalor]

On Thu, 23 Jan 2020 at 15:32, Mahendra Singh Thalor  wrote:
>
> On Wed, 22 Jan 2020 at 12:48, Masahiko Sawada
>  wrote:
> >
> > On Wed, 22 Jan 2020 at 11:23, Amit Kapila  wrote:
> > >
> > > On Wed, Jan 22, 2020 at 7:14 AM Masahiko Sawada
> > >  wrote:
> > > >
> > > > Thank you for updating the patch. Yeah MAXDEADTUPLES is better than
> > > > what I did in the previous version patch.
> > > >
> > >
> > > Would you like to resubmit your vacuumdb utility patch for this
> > > enhancement?  I see some old version of it and it seems to me that you
> > > need to update that patch.
> > >
> > > + if (optarg != NULL)
> > > + {
> > > + parallel_workers = atoi(optarg);
> > > + if (parallel_workers <= 0)
> > > + {
> > > + pg_log_error("number of parallel workers must be at least 1");
> > > + exit(1);
> > > + }
> > > + }
> > >
> > > This will no longer be true.
> >
> > Attached the updated version patch.
> >
>
> Thanks Sawada-san for the re-based patch.
>
> I reviewed and tested this patch.  Patch looks good to me.

As offline, suggested by Amit Kapila, I verified vacuumdb "-P" option
functionality with older versions(<13) and also I tested vacuumdb by
giving "-j" option with "-P". All are working as per expectation and I
didn't find any issue with these options.

-- 
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com

Re: Allow to_date() and to_timestamp() to accept localized names

[Alvaro Herrera]

On 2020-Jan-24, Juan José Santamaría Flecha wrote:

> There is an open patch that will make the normalization functionality user
> visible [1]. So, if a user can call to_date(normalize('01 ŞUB 2010'), 'DD
> TMMON ') I would vote to drop the normalization logic inside this patch
> altogether.

I was reading the SQL standard on this point, and it says this (4.2.8
Universal character sets):

  An SQL-implementation may assume that all UCS strings are normalized
  in one of [Unicode normalization forms].

which seems to agree with what you're saying.

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

Re: Add pg_file_sync() to adminpack

[Fujii Masao]

On 2020/01/24 17:08, Fujii Masao wrote:



On 2020/01/24 16:56, Julien Rouhaud wrote:
On Fri, Jan 24, 2020 at 8:20 AM Fujii Masao 
 wrote:


On 2020/01/24 15:38, Arthur Zakirov wrote:

On 2020/01/24 14:56, Michael Paquier wrote:

On Fri, Jan 24, 2020 at 01:28:29PM +0900, Arthur Zakirov wrote:

It is compiled and passes the tests. There is the documentation and
it is
built too without an error.

It seems that consensus about the returned type was reached and I
marked the
patch as "Ready for Commiter".


+   fsync_fname_ext(filename, S_ISDIR(fst.st_mode), false, ERROR);
One comment here: should we warn better users in the docs that a fsync
failule will not trigger a PANIC here?  Here, fsync failure on heap
file => ERROR => potential data corruption.


Ah, true. It is possible to add couple sentences that pg_file_sync()
doesn't depend on data_sync_retry GUC and doesn't raise a PANIC even 
for

database files.


Thanks all for the review!

So, what about the attached patch?
In the patch, I added the following note to the doc.


Note that
 has no effect on this function,
and therefore a PANIC-level error will not be raised even on failure to
flush database files.



We should explicitly mention that this can cause corruption.  How  about:


Note that
 has no effect on this function,
and therefore a PANIC-level error will not be raised even on failure to
flush database files.  If that happens, the underlying database
objects may be corrupted.



IMO that's overkill. If we really need such mention for pg_file_sync(),
we also need to add it for other functions like pg_read_file(),
pg_stat_file(), etc. But, again, which looks overkill.


I pushed the v5 of the patch. Thanks all for reviewing the patch!
If the current document is not good yet, let's keep discussing that!

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

Re: [Proposal] Global temporary tables

[Pavel Stehule]

pá 24. 1. 2020 v 10:43 odesílatel Konstantin Knizhnik <
k.knizh...@postgrespro.ru> napsal:

>
>
> On 24.01.2020 12:09, Pavel Stehule wrote:
>
>
>
> pá 24. 1. 2020 v 9:39 odesílatel Konstantin Knizhnik <
> k.knizh...@postgrespro.ru> napsal:
>
>>
>>
>> On 23.01.2020 23:47, Robert Haas wrote:
>> > On Sat, Jan 11, 2020 at 8:51 PM Tomas Vondra
>> >  wrote:
>> >> I proposed just ignoring those new indexes because it seems much
>> simpler
>> >> than alternative solutions that I can think of, and it's not like those
>> >> other solutions don't have other issues.
>> > +1.
>> >
>> >> For example, I've looked at the "on demand" building as implemented in
>> >> global_private_temp-8.patch, I kinda doubt adding a bunch of index
>> build
>> >> calls into various places in index code seems somewht suspicious.
>> > +1. I can't imagine that's a safe or sane thing to do.
>> >
>>
>> As far as you know there are two versions of GTT implementations now.
>> And we are going to merge them into single patch.
>> But there are some principle question concerning provided functionality
>> which has to be be discussed:
>> should we prohibit DDL on GTT if there are more than one sessions using
>> it. It includes creation/dropping indexes, dropping table, altering
>> table...
>>
>> If the answer is "yes", then the question whether to populate new
>> indexes with data is no relevant at all, because such situation will not
>> be possible.
>> But in this case we will get incompatible behavior with normal
>> (permanent) tables and it seems to be very inconvenient from DBA point
>> of view:
>> it will be necessary to enforce all clients to close their sessions to
>> perform some DDL manipulations with GTT.
>> Some DDLs will be very difficult to implement if GTT is used by more
>> than one backend, for example altering table schema.
>>
>> My current solution is to allow creation/droping index on GTT and
>> dropping table itself, while prohibit alter schema at all for GTT.
>> Wenjing's approach is to prohibit any DDL if GTT is used by more than
>> one backend.
>>
>
> When I create index on GTT in one session, then I don't expect creating
> same index in all other sessions that uses same GTT.
>
> But I can imagine to creating index on GTT enforces index in current
> session, and for other sessions this index will be invalid to end of
> session.
>
>
> So there are three possible alternatives:
>
> 1. Prohibit index creation of GTT when it used by more than once session.
> 2. Create index and populate them with data in all sessions using this GTT.
> 3. Create index only in current session and do not allow to use it in all
> other sessions already using this GTT (but definitely allow to use it in
> new sessions).
>
> 1 is Wenjing's approach, 2 - is my approach, 3 - is your suggestion :)
>
> I can construct the following table with pro/cons of each approach:
>
> Approach
> Compatibility with normal table
> User (DBA) friendly
> Complexity of implementation
> Consistency
> 1
> -
> 1: requires restart of all sessions to perform operation
> 2: requires global cache of GTT
> 3*: *no man, no problem
> 2
> +
> 3: if index is created then it is actually needed, isn't it? 1: use
> existed functionality to create index
> 2: if alter schema is prohibited
> 3
> -
> 2: requires restart of all sessions to use created index
> 3: requires some mechanism for prohibiting index created after first
> session access to GTT
> 1: can perform DDL but do no see effect of it
>
>
You will see a effect of DDL in current session (where you did the change),
all other sessions should to live without any any change do reconnect or to
RESET connect

I don't like 2 - when I do index on global temp table, I don't would to
wait on indexing on all other sessions. These operations should be
maximally independent.

Regards

Pavel


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

Re: [PATCH] Windows port, fix some resources leaks

[Ranier Vilela]

Em sex., 24 de jan. de 2020 às 04:13, Michael Paquier 
escreveu:

> On Wed, Jan 22, 2020 at 05:51:51PM -0300, Ranier Vilela wrote:
> > After review the patches and build all and run regress checks for each
> > patch, those are the ones that don't break.
>
> There is some progress.  You should be careful about your patches,
> as they generate compiler warnings.  Here is one quote from gcc-9:
> logging.c:87:13: warning: passing argument 1 of ‘free’ discards
> ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
>87 |free(sgr_warning);
>
Well, in this cases, the solution is cast.
free((char *) sgr_warning);

But there are others.
>
> if (strcmp(name, "error") == 0)
> +   {
> +   free(sgr_error);
> sgr_error = strdup(value);
> +   }
> I don't see the point of doing that in logging.c.  pg_logging_init()
> is called only once per tools, so this cannot happen.  Another point
> that may matter here though is that we do not complain about OOMs.
> That's really unlikely to happen, and if it happens it leads to
> partially colored output.
>
Coverity show the alert, because he tries all the possibilites.Is inside a
loop.
It seems to me that the only way to happen is by the user, by introducing a
repeated and wrong sequence.
If ok, we can discard this patch, but free doens't hurt here.


> -   NOERR();
> +   if (ISERR())
> +   {
> +   freedfa(s);
> +   return v->err;
> +   }
> Can you design a query where this is a problem?
>
 I think for now, I’m not able to do it.
But, the fix is better do not you think.
The macro hides the return and the exchange does not change the final size.
If the ISERR() it never occurs here, nor would we need the macro.

pg_log_error("could not allocate SIDs: error code %lu",
> GetLastError());
> +   CloseHandle(origToken);
> +   FreeLibrary(Advapi32Handle);
> [...]
> pg_log_error("could not open process token: error code %lu",
> GetLastError());
> +   FreeLibrary(Advapi32Handle);
> return 0;
> For those two ones, it looks that you are right.  However, I think
> that it would be safer to check if Advapi32Handle is NULL for both.
>
Michael, I did it differently and modified the function to not need to test
NULL, I think it was better.

 @@ -187,6 +190,7 @@ get_restricted_token(void)

> }
> exit(x);
> }
> +   free(cmdline);
> Anything allocated with pg_strdup() should be free'd with pg_free(),
> that's a matter of consistency.
>
Done.


> +++ b/src/backend/postmaster/postmaster.c
> @@ -4719,6 +4719,8 @@ retry:
> if (cmdLine[sizeof(cmdLine) - 2] != '\0')
> {
> elog(LOG, "subprocess command line too long");
> +   UnmapViewOfFile(param);
> +   CloseHandle(paramHandle);
> The three ones in postmaster.c are correct guesses.
>
> Does that mean it is correct?


> +   if (sspictx != NULL)
> +   {
> +   DeleteSecurityContext(sspictx);
> +   free(sspictx);
> +   }
> +   FreeCredentialsHandle(&sspicred);
> This stuff is correctly free'd after calling AcceptSecurityContext()
> in the SSPI code, but not the two other code paths.  Looks right.
> Actually, for the first one, wouldn't it be better to free those
> resources *before* ereport(ERROR) on ERRCODE_PROTOCOL_VIOLATION?
> That's an authentication path so it does not really matter but..
>
 Done.


> ldap_unbind(*ldap);
> +   FreeLibrary(ldaphandle);
> return STATUS_ERROR;
> Yep.  That's consistent to clean up.
>
Ok.

>
> +   if (VirtualFree(ShmemProtectiveRegion, 0, MEM_RELEASE) == 0)
> +   elog(FATAL, "failed to release reserved memory region
> (addr=%p): error code %lu",
> +   ShmemProtectiveRegion, GetLastError());
> return false;
> No, that's not right.  I think that it is possible to loop over
> ShmemProtectiveRegion in some cases.  And actually, your patch is dead
> wrong because this is some code called by the postmaster and it cannot
> use FATAL.
>
FATAL changed to LOG, you are right.
In case of loop, VirtualAllocEx wouldn't be called again?


> > Not all leaks detected by Coverity are fixed.
>
> Coverity is a static analyzer, it misses a lot of things tied to the
> context of the code, so you need to take its suggestions with a pinch
> of salt.
>
Oh yes, true.
I think that all alerts are true, because they test all possibilities, even
those that are rarely, or almost impossible to happen.

Thank you for the review.

Best regards,
Ranier Vilela


auth_leak.patch
Description: Binary data


postmaster_leak.patch
Description: Binary data


regex_leak.patch
Description: Binary data


restricted_token_leaks.patch
Description: Binary data

Re: [PATCH] Windows port, fix some resources leaks

[Ranier Vilela]

Last time improvement to restricted_token.c

regards,
Ranier Vilela


restricted_token_leaks.patch
Description: Binary data

Re: [PoC] Non-volatile WAL buffer

[Heikki Linnakangas]

On 24/01/2020 10:06, Takashi Menjo wrote:

I propose "non-volatile WAL buffer," a proof-of-concept new feature.  It
enables WAL records to be durable without output to WAL segment files by
residing on persistent memory (PMEM) instead of DRAM.  It improves database
performance by reducing copies of WAL and shortening the time of write
transactions.

I attach the first patchset that can be applied to PostgreSQL 12.0 (refs/
tags/REL_12_0).  Please see README.nvwal (added by the patch 0003) to use
the new feature.


I have the same comments on this that I had on the previous patch, see:

https://www.postgresql.org/message-id/2aec6e2a-6a32-0c39-e4e2-aad854543aa8%40iki.fi

- Heikki

Re: [Proposal] Global temporary tables

[Konstantin Knizhnik]

On 24.01.2020 15:15, Pavel Stehule wrote:
You will see a effect of DDL in current session (where you did the 
change), all other sessions should to live without any any change do 
reconnect or to RESET connect


Why? I found this requirement quit unnatural and contradicting to the 
behavior of normal tables.
Actually one of motivation for adding global tempo tables to Postgres is 
to provide compatibility with Oracle.
Although I know that Oracle design decisions were never considered as  
axioms by Postgres community,
but ni case of GTT design I think that we should take in account Oracle 
approach.

And GTT in Oracle behaves exactly as in my implementation:

https://www.oracletutorial.com/oracle-basics/oracle-global-temporary-table/

It is not clear from this documentation whether index created for GTT in 
one session can be used in another session which already has some data 
in this GTT.
But I did experiment with install Oracle server and  can confirm that 
actually works in this way.


So I do not understand why do we need to complicate our GTT 
implementation in order to prohibit useful functionality and introduce 
inconsistency between behavior of normal and global temp tables.




I don't like 2 - when I do index on global temp table, I don't would 
to wait on indexing on all other sessions. These operations should be 
maximally independent.




Nobody suggest to wait building index in all sessions.
Indexes will be constructed on demand when session access this table.
If session will no access this table at all, then index will never be 
constructed.


Once again: logic of dealing with indexes in GTT is very simple.
For normal tables, indexes are initialized at the tame when them are 
created.
For GTT it is not true. We have to initialize index on demand when it is 
accessed first time in session.


So it has to be handled in any way.
The question is only whether we should allow creation of index for table 
already populated with some data?
Actually doesn't require some additional efforts. We can use existed 
build_index function which initialize index and populates it with data.
So the solution proposed for me is most natural, convenient and simplest 
solution at the same time. And compatible with Oracle.






Regards

Pavel



-- 
Konstantin Knizhnik

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



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

Re: Thoughts on "killed tuples" index hint bits support on standby

[Michail Nikolaev]

Hello again.

Andres, Peter, thanks for your comments.

Some of issues your mentioned (reporting feedback to the another
cascade standby, processing queries after restart and newer xid
already reported) could be fixed in provided design, but your
intention to have "independent correctness backstop" is a right thing
to do.

So, I was thinking about another approach which is:
* still not too tricky to implement
* easy to understand
* does not rely on hot_standby_feedback for correctness, but only for efficiency
* could be used with any kind of index
* does not generate a lot of WAL

Let's add a new type of WAL record like "some index killed tuple hint
bits are set according to RecentGlobalXmin=x" (without specifying page
or even relation). Let's call 'x' as 'LastKilledIndexTuplesXmin' and
track it in standby memory. It is sent only in case of
wal_log_hints=true. If hints cause FPW - it is sent before FPW record.
Also, it is not required to write such WAL every time primary marks
index tuple as dead. It should be done only in case
'LastKilledIndexTuplesXmin' is changed (moved forward).

On standby such record is used to cancel queries. If transaction is
executed with "ignore_killed_tuples==true" (set on snapshot creation)
and its xid is less than received LastKilledIndexTuplesXmin - just
cancel the query (because it could rely on invalid hint bit). So,
technically it should be correct to use hints received from master to
skip tuples according to MVCC, but "the conflict rate goes through the
roof".

To avoid any real conflicts standby sets
ignore_killed_tuples = (hot_standby_feedback is on)
   AND (wal_log_hints is on on primary)
   AND (standby new snapshot xid >= last
LastKilledIndexTuplesXmin received)
   AND (hot_standby_feedback is reported
directly to master).

So, hot_standby_feedback loop effectively eliminates any conflicts
(because LastKilledIndexTuplesXmin is technically RecentGlobalXmin in
such case). But if feedback is broken for some reason - query
cancellation logic will keep everything safe.

For correctness LastKilledIndexTuplesXmin (and as consequence
RecentGlobalXmin) should be moved only forward.

To set killed bits on standby we should check tuples visibility
according to last LastKilledIndexTuplesXmin received. It is just like
master sets these bits according to its state - so it is even safe to
transfer them to another standby.

Does it look better now?

Thanks, Michail.

Re: making the backend's json parser work in frontend code

[Mark Dilger]

> On Jan 23, 2020, at 4:27 PM, Andrew Dunstan  
> wrote:
> 
> On Fri, Jan 24, 2020 at 7:35 AM Mark Dilger
>  wrote:
>> 
>> 
>>> On Jan 22, 2020, at 7:00 PM, Mark Dilger  
>>> wrote:
>>> 
>>> I have this done in my local repo to the point that I can build frontend 
>>> tools against the json parser that is now in src/common and also run all 
>>> the check-world tests without failure.  I’m planning to post my work soon, 
>>> possibly tonight if I don’t run out of time, but more likely tomorrow.
>> 
>> Ok, I finished merging with Robert’s patches.  The attached follow his 
>> numbering, with my patches intended to by applied after his.
>> 
>> I tried not to change his work too much, but I did a bit of refactoring in 
>> 0010, as explained in the commit comment.
>> 
>> 0011 is just for verifying the linking works ok and the json parser can be 
>> invoked from a frontend tool without error — I don’t really see the point in 
>> committing it.
>> 
>> I ran some benchmarks for json parsing in the backend both before and after 
>> these patches, with very slight changes in runtime.  The setup for the 
>> benchmark creates an unlogged table with a single text column and loads rows 
>> of json formatted text:
>> 
>> CREATE UNLOGGED TABLE benchmark (
>>j text
>> );
>> COPY benchmark (j) FROM '/Users/mark.dilger/bench/json.csv’;
>> 
>> 
>> FYI:
>> 
>> wc ~/bench/json.csv
>> 107 34465023 503364244 /Users/mark.dilger/bench/json.csv
>> 
>> The benchmark itself casts the text column to jsonb, as follows:
>> 
>> SELECT jsonb_typeof(j::jsonb) typ, COUNT(*) FROM benchmark GROUP BY typ;
>> 
>> In summary, the times are:
>> 
>>pristinepatched
>>—   —
>>11.985  12.237
>>12.200  11.992
>>11.691  11.896
>>11.847  11.833
>>11.722  11.936
>> 
> 
> OK, nothing noticeable there.
> 
> "accept" is a common utility I've used in the past with parsers of
> this kind, but inlining it seems perfectly reasonable.
> 
> I've reviewed these patches and Robert's, and they seem basically good to me.

Thanks for the review!

> But I don't think src/bin is the right place for the test program. I
> assume we're not going to ship this program, so it really belongs in
> src/test somewhere, I think. It should also have a TAP test.

Ok, I’ll go do that; thanks for the suggestion.


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: progress report for ANALYZE

[Amit Langote]

On Fri, Jan 24, 2020 at 6:47 AM Alvaro Herrera  wrote:
> On 2020-Jan-22, Tatsuro Yamada wrote:
> > P.S.
> > Next up is progress reporting for query execution?!
>
> Actually, I think it's ALTER TABLE.

+1.  Existing infrastructure might be enough to cover ALTER TABLE's
needs, whereas we will very likely need to build entirely different
infrastructure for tracking the progress for SQL query execution.

Thanks,
Amit

Re: making the backend's json parser work in frontend code

[Peter Eisentraut]

On 2020-01-23 18:04, Robert Haas wrote:

Now, you might say "well, why don't we just do an encoding
conversion?", but we can't. When the filesystem tells us what the file
names are, it does not tell us what encoding the person who created
those files had in mind. We don't know that they had*any*  encoding in
mind. IIUC, a file in the data directory can have a name that consists
of any sequence of bytes whatsoever, so long as it doesn't contain
prohibited characters like a path separator or \0 byte. But only some
of those possible octet sequences can be stored in a manifest that has
to be valid UTF-8.


I think it wouldn't be unreasonable to require that file names in the 
database directory be consistently encoded (as defined by pg_control, 
probably).  After all, this information is sometimes also shown in 
system views, so it's already difficult to process total junk.  In 
practice, this shouldn't be an onerous requirement.


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

Re: Allow to_date() and to_timestamp() to accept localized names

[Tom Lane]

=?UTF-8?Q?Juan_Jos=C3=A9_Santamar=C3=ADa_Flecha?=  
writes:
> On Thu, Jan 23, 2020 at 11:00 PM Tom Lane  wrote:
>> * It's not exactly apparent to me why this code should be concerned
>> about non-normalized characters when noplace else in the backend is.

> There is an open patch that will make the normalization functionality user
> visible [1]. So, if a user can call to_date(normalize('01 ŞUB 2010'), 'DD
> TMMON ') I would vote to drop the normalization logic inside this patch
> altogether.

Works for me.

> * I have no faith in this calculation that decides how long the match
>> length was:
>>  *len = element_len + name_len - norm_len;

> The proper logic would come from do_to_timestamp() receiving a normalized
> "date_txt" input, so we do not operate with unnormalize and normalize
> strings at the same time.

No, that only solves half the problem, because the downcasing
transformation can change the string length too.  Two easy examples:

* In German, I believe "ß" downcases to "ss".  In Latin-1 encoding that's
a length change, though I think it might accidentally not be in UTF8.

* The Turks distinguish dotted and dotless "i", so that "İ" downcases to
"i", and conversely "I" downcases to "ı".  Those are length changes in
UTF8, though not in whichever Latin-N encoding works for Turkish.

Even if these cases happen not to apply to any month or day name of
the relevant language, we still have a problem, arising from the fact
that we're downcasing the whole remaining string --- so length changes
after the match could occur anyway.

> I would like to rise a couple of questions myself:

> * When compiled with DEBUG_TO_FROM_CHAR, there is a warning "‘dump_node’
> defined but not used". Should we drop this function or uncomment its usage?

Maybe, but I don't think it belongs in this patch.

> * Would it be worth moving str_tolower(localized_name)
> from seq_search_localized() into cache_locale_time()?

I think it'd complicate tracking when that cache has to be invalidated
(i.e. it now depends on more than just LC_TIME).  On the whole I wouldn't
bother unless someone does the measurements to show there'd be a useful
speedup.

regards, tom lane

Re: Busted(?) optimization in ATAddForeignKeyConstraint

[Tom Lane]

Peter Eisentraut  writes:
> On 2020-01-23 23:11, Tom Lane wrote:
>> I happened to notice this comment in the logic in
>> ATAddForeignKeyConstraint that tries to decide if it can skip
>> revalidating a foreign-key constraint after a DDL change:
>>  * Since we require that all collations share the same notion of
>>  * equality (which they do, because texteq reduces to bitwise
>>  * equality), we don't compare collation here.
>> Hasn't this been broken by the introduction of nondeterministic
>> collations?

> I'm not very familiar with the logic in this function, but I think this 
> might be okay because the foreign-key equality comparisons are done with 
> the collation of the primary key, which doesn't change here AFAICT.

If we're depending on that, we should just remove the comment and compare
the collations.  Seems far less likely to break.

Even if there's a reason not to do the comparison, the comment needs
an update.

regards, tom lane

Re: Allow to_date() and to_timestamp() to accept localized names

[Peter Eisentraut]

On 2020-01-24 12:44, Alvaro Herrera wrote:

On 2020-Jan-24, Juan José Santamaría Flecha wrote:


There is an open patch that will make the normalization functionality user
visible [1]. So, if a user can call to_date(normalize('01 ŞUB 2010'), 'DD
TMMON ') I would vote to drop the normalization logic inside this patch
altogether.


I was reading the SQL standard on this point, and it says this (4.2.8
Universal character sets):

   An SQL-implementation may assume that all UCS strings are normalized
   in one of [Unicode normalization forms].

which seems to agree with what you're saying.


When you interact with glibc locale functions, you essentially have to 
assume that everything is in NFC.  When using ICU locale functions 
(which we don't here, but just for the sake of argument), then I would 
expect that ICU does appropriate normalization itself when I call 
icu_what_month_is_this(string) returns int.  So I think it is 
appropriate to not deal with normalization in this patch.


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

Re: Error message inconsistency

[Tom Lane]

Amit Kapila  writes:
> LGTM.  I have combined them into the single patch.  What do we think
> about backpatching this?

No objection to the patch for HEAD, but it does not seem like
back-patch material: it is not fixing a bug.

regards, tom lane

Re: libxml2 is dropping xml2-config

[David Steele]

On 1/23/20 7:09 AM, Christoph Berg wrote:
> Re: David Steele 2020-01-21 
<95349047-31dd-c7dc-df17-b488c2d34...@pgmasters.net>
>> Yes -- at least Ubuntu < 18.04 does not install pkg-config for 
libxml2. I

>> have not checked Debian yet, but I imagine < 8 will have the same issue.
>
> That is not true, I just verified that both 16.04 and 14.04 (already
> EOL) have a working `pkg-config libxml-2.0 --libs`.

You are correct.  My build script was not explicitly installing 
pkg-config on <= 16.04.  12.04 also seems to work fine.


Regards,
--
-David
da...@pgmasters.net

Re: Allow to_date() and to_timestamp() to accept localized names

[Alvaro Herrera]

On 2020-Jan-24, Peter Eisentraut wrote:

> When you interact with glibc locale functions, you essentially have to
> assume that everything is in NFC.  When using ICU locale functions (which we
> don't here, but just for the sake of argument), then I would expect that ICU
> does appropriate normalization itself when I call
> icu_what_month_is_this(string) returns int.  So I think it is appropriate to
> not deal with normalization in this patch.

But that's a different POV.  The input to this function could come from
arbitrary user input from any application whatsoever.  So the only
reason we can get away with that is because the example regression case
Juan José added (which uses non-normals) does not conform to the
standard.

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

Re: Allow to_date() and to_timestamp() to accept localized names

[Tom Lane]

Alvaro Herrera  writes:
> But that's a different POV.  The input to this function could come from
> arbitrary user input from any application whatsoever.  So the only
> reason we can get away with that is because the example regression case
> Juan José added (which uses non-normals) does not conform to the
> standard.

I'm unsure about "conforming to standard", but I think it's reasonable
to put the onus of doing normalization when necessary on the user.
Otherwise, we need to move normalization logic into basically all
the string processing functions (even texteq), which seems like a
pretty huge cost that will benefit only a small minority of people.
(If it's not a small minority, then where's the bug reports complaining
that we don't do it today?)

regards, tom lane

Re: making the backend's json parser work in frontend code

[Tom Lane]

Peter Eisentraut  writes:
> On 2020-01-23 18:04, Robert Haas wrote:
>> Now, you might say "well, why don't we just do an encoding
>> conversion?", but we can't. When the filesystem tells us what the file
>> names are, it does not tell us what encoding the person who created
>> those files had in mind. We don't know that they had*any*  encoding in
>> mind. IIUC, a file in the data directory can have a name that consists
>> of any sequence of bytes whatsoever, so long as it doesn't contain
>> prohibited characters like a path separator or \0 byte. But only some
>> of those possible octet sequences can be stored in a manifest that has
>> to be valid UTF-8.

> I think it wouldn't be unreasonable to require that file names in the 
> database directory be consistently encoded (as defined by pg_control, 
> probably).  After all, this information is sometimes also shown in 
> system views, so it's already difficult to process total junk.  In 
> practice, this shouldn't be an onerous requirement.

I don't entirely follow why we're discussing this at all, if the
requirement is backing up a PG data directory.  There are not, and
are never likely to be, any legitimate files with non-ASCII names
in that context.  Why can't we just skip any such files?

regards, tom lane

Re: making the backend's json parser work in frontend code

[David Steele]

On 1/23/20 11:05 AM, Robert Haas wrote:
> On Thu, Jan 23, 2020 at 12:49 PM Bruce Momjian  wrote:
>> Another idea is to use base64 for all non-ASCII file names, so we don't
>> need to check if the file name is valid UTF8 before outputting --- we
>> just need to check for non-ASCII, which is much easier.
>
> I think that we have the infrastructure available to check in a
> convenient way whether it's valid as UTF-8, so this might not be
> necessary, but I will look into it further unless there is a consensus
> to go another direction entirely.
>
>> Another
>> problem, though, is how do you _flag_ file names as being
>> base64-encoded?  Use another JSON field to specify that?
>
> Alvaro's proposed solution in the message to which you replied was to
> call the field either 'path' or 'path_base64' depending on whether
> base-64 escaping was used. That seems better to me than having a field
> called 'path' and a separate field called 'is_path_base64' or
> whatever.

+1. I'm not excited about this solution but don't have a better idea.

It might be nice to have a strict mode where non-ASCII/UTF8 characters 
will error instead, but that can be added on later.


Regards,
--
-David
da...@pgmasters.net

Re: making the backend's json parser work in frontend code

[David Steele]

On 1/24/20 9:27 AM, Tom Lane wrote:

Peter Eisentraut  writes:

On 2020-01-23 18:04, Robert Haas wrote:

Now, you might say "well, why don't we just do an encoding
conversion?", but we can't. When the filesystem tells us what the file
names are, it does not tell us what encoding the person who created
those files had in mind. We don't know that they had*any*  encoding in
mind. IIUC, a file in the data directory can have a name that consists
of any sequence of bytes whatsoever, so long as it doesn't contain
prohibited characters like a path separator or \0 byte. But only some
of those possible octet sequences can be stored in a manifest that has
to be valid UTF-8.



I think it wouldn't be unreasonable to require that file names in the
database directory be consistently encoded (as defined by pg_control,
probably).  After all, this information is sometimes also shown in
system views, so it's already difficult to process total junk.  In
practice, this shouldn't be an onerous requirement.


I don't entirely follow why we're discussing this at all, if the
requirement is backing up a PG data directory.  There are not, and
are never likely to be, any legitimate files with non-ASCII names
in that context.  Why can't we just skip any such files?


It's not uncommon in my experience for users to drop odd files into 
PGDATA (usually versioned copies of postgresql.conf, etc.), but I agree 
that it should be discouraged.  Even so, I don't recall ever seeing any 
non-ASCII filenames.


Skipping files sounds scary, I'd prefer an error or a warning (and then 
base64 encode the filename).


Regards,
--
-David
da...@pgmasters.net

Re: Allow to_date() and to_timestamp() to accept localized names

[Peter Eisentraut]

On 2020-01-24 17:22, Tom Lane wrote:

Alvaro Herrera  writes:

But that's a different POV.  The input to this function could come from
arbitrary user input from any application whatsoever.  So the only
reason we can get away with that is because the example regression case
Juan José added (which uses non-normals) does not conform to the
standard.


I'm unsure about "conforming to standard", but I think it's reasonable
to put the onus of doing normalization when necessary on the user.
Otherwise, we need to move normalization logic into basically all
the string processing functions (even texteq), which seems like a
pretty huge cost that will benefit only a small minority of people.
(If it's not a small minority, then where's the bug reports complaining
that we don't do it today?)


These reports do exist, and this behavior is known.  However, the impact 
is mostly that results "look wrong" (looks the same but doesn't compare 
as equal) rather than causing inconsistency and corruption, so it's 
mostly shrugged off.  The nondeterministic collation feature was 
introduced in part to be able to deal with this; the pending 
normalization patch is another.  However, this behavior is baked deeply 
into Unicode, so no single feature or facility will simply make it go away.


AFAICT, we haven't so far had any code that does a lookup of non-ASCII 
strings in a table, so that's why we haven't had this discussion yet.


Now that I think about it, you could also make an argument that this 
should be handled through collation, so the function that looks up the 
string in the locale table should go through texteq.  However, this 
would mostly satisfy the purists but create a bizarre user experience.


Looking through the patch quickly, if you want to get Unicode-fancy, 
doing a case-insensitive comparison by running lower-case on both 
strings is also wrong in corner cases.  All the Greek month names end in 
sigma, so I suspect that this patch might not work correctly in such cases.


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

Re: Allow to_date() and to_timestamp() to accept localized names

[Tom Lane]

Peter Eisentraut  writes:
> Looking through the patch quickly, if you want to get Unicode-fancy, 
> doing a case-insensitive comparison by running lower-case on both 
> strings is also wrong in corner cases.  All the Greek month names end in 
> sigma, so I suspect that this patch might not work correctly in such cases.

Hm.  That's basically what citext does, and I don't recall hearing
complaints about that.  What other definition of "case insensitive"
would you suggest?

regards, tom lane

Re: making the backend's json parser work in frontend code

[Alvaro Herrera]

On 2020-Jan-24, David Steele wrote:

> It might be nice to have a strict mode where non-ASCII/UTF8 characters will
> error instead, but that can be added on later.

"your backup failed because you have a file we don't like" is not great
behavior.  IIRC we already fail when a file is owned by root (or maybe
unreadable and owned by root), and it messes up severely when people
edit postgresql.conf as root.  Let's not add more cases of that sort.

Maybe we can get away with *ignoring* such files, perhaps after emitting
a warning.

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

Re: Duplicate Workers entries in some EXPLAIN plans

[Maciek Sakrejda]

Great, thank you. I noticed in the CF patch tester app, the build
fails on Windows [1]. Investigating, I realized I had failed to fully
strip volatile EXPLAIN info (namely buffers) in TEXT mode due to a
bad regexp_replace. I've fixed this in the attached patch (which I
also rebased against current master again).

[1]: https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.76313
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index d189b8d573..b4108f82ec 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -100,7 +100,7 @@ static void show_sortorder_options(StringInfo buf, Node *sortexpr,
    Oid sortOperator, Oid collation, bool nullsFirst);
 static void show_tablesample(TableSampleClause *tsc, PlanState *planstate,
 			 List *ancestors, ExplainState *es);
-static void show_sort_info(SortState *sortstate, ExplainState *es);
+static void show_sort_info(SortState *sortstate, StringInfo* worker_strs, ExplainState *es);
 static void show_hash_info(HashState *hashstate, ExplainState *es);
 static void show_tidbitmap_info(BitmapHeapScanState *planstate,
 ExplainState *es);
@@ -131,7 +131,9 @@ static void ExplainXMLTag(const char *tagname, int flags, ExplainState *es);
 static void ExplainJSONLineEnding(ExplainState *es);
 static void ExplainYAMLLineStarting(ExplainState *es);
 static void escape_yaml(StringInfo buf, const char *str);
-
+static void ExplainOpenWorker(StringInfo worker_str, ExplainState *es);
+static void ExplainCloseWorker(ExplainState *es);
+static void ExplainFlushWorkers(StringInfo *worker_strs, int num_workers, ExplainState *es);
 
 
 /*
@@ -289,6 +291,7 @@ NewExplainState(void)
 	es->costs = true;
 	/* Prepare output buffer. */
 	es->str = makeStringInfo();
+	es->root_str = es->str;
 
 	return es;
 }
@@ -1090,9 +1093,19 @@ ExplainNode(PlanState *planstate, List *ancestors,
 	const char *partialmode = NULL;
 	const char *operation = NULL;
 	const char *custom_name = NULL;
+	StringInfo *worker_strs = NULL;
 	int			save_indent = es->indent;
 	bool		haschildren;
 
+	/* Prepare per-worker output */
+	if (es->analyze && planstate->worker_instrument)
+	{
+		int num_workers = planstate->worker_instrument->num_workers;
+		worker_strs = (StringInfo *) palloc0(num_workers * sizeof(StringInfo));
+		for (int n = 0; n < num_workers; n++)
+			worker_strs[n] = makeStringInfo();
+	}
+
 	switch (nodeTag(plan))
 	{
 		case T_Result:
@@ -1357,6 +1370,55 @@ ExplainNode(PlanState *planstate, List *ancestors,
 		ExplainPropertyBool("Parallel Aware", plan->parallel_aware, es);
 	}
 
+	/* Prepare worker general execution details */
+	if (es->analyze && es->verbose && !es->hide_workers && planstate->worker_instrument)
+	{
+		WorkerInstrumentation *w = planstate->worker_instrument;
+
+		for (int n = 0; n < w->num_workers; ++n)
+		{
+			Instrumentation *instrument = &w->instrument[n];
+			double		nloops = instrument->nloops;
+			double		startup_ms;
+			double		total_ms;
+			double		rows;
+
+			if (nloops <= 0)
+continue;
+			startup_ms = 1000.0 * instrument->startup / nloops;
+			total_ms = 1000.0 * instrument->total / nloops;
+			rows = instrument->ntuples / nloops;
+
+			ExplainOpenWorker(worker_strs[n], es);
+
+			if (es->format == EXPLAIN_FORMAT_TEXT)
+			{
+if (es->timing)
+	appendStringInfo(es->str,
+	 "actual time=%.3f..%.3f rows=%.0f loops=%.0f\n",
+	 startup_ms, total_ms, rows, nloops);
+else
+	appendStringInfo(es->str,
+	 "actual rows=%.0f loops=%.0f\n",
+	 rows, nloops);
+			}
+			else
+			{
+if (es->timing)
+{
+	ExplainPropertyFloat("Actual Startup Time", "ms",
+		 startup_ms, 3, es);
+	ExplainPropertyFloat("Actual Total Time", "ms",
+		 total_ms, 3, es);
+}
+ExplainPropertyFloat("Actual Rows", NULL, rows, 0, es);
+ExplainPropertyFloat("Actual Loops", NULL, nloops, 0, es);
+			}
+		}
+
+		ExplainCloseWorker(es);
+	}
+
 	switch (nodeTag(plan))
 	{
 		case T_SeqScan:
@@ -1856,7 +1918,7 @@ ExplainNode(PlanState *planstate, List *ancestors,
 			break;
 		case T_Sort:
 			show_sort_keys(castNode(SortState, planstate), ancestors, es);
-			show_sort_info(castNode(SortState, planstate), es);
+			show_sort_info(castNode(SortState, planstate), worker_strs, es);
 			break;
 		case T_MergeAppend:
 			show_merge_append_keys(castNode(MergeAppendState, planstate),
@@ -1885,76 +1947,31 @@ ExplainNode(PlanState *planstate, List *ancestors,
 	if (es->buffers && planstate->instrument)
 		show_buffer_usage(es, &planstate->instrument->bufusage);
 
-	/* Show worker detail */
-	if (es->analyze && es->verbose && !es->hide_workers &&
-		planstate->worker_instrument)
+	/* Prepare worker buffer usage */
+	if (es->buffers && es->analyze && es->verbose && !es->hide_workers
+		&& planstate->worker_instrument)
 	{
 		WorkerInstrumentation *w = planstate->worker_instrument;
-		bool		opened_group = false;
 		int			n;
 
 		for (n = 0; n < w->num_workers

Re: making the backend's json parser work in frontend code

[David Steele]

On 1/24/20 10:00 AM, Alvaro Herrera wrote:

On 2020-Jan-24, David Steele wrote:


It might be nice to have a strict mode where non-ASCII/UTF8 characters will
error instead, but that can be added on later.


"your backup failed because you have a file we don't like" is not great
behavior.  IIRC we already fail when a file is owned by root (or maybe
unreadable and owned by root), and it messes up severely when people
edit postgresql.conf as root.  Let's not add more cases of that sort.


My intention was that the strict mode would not be the default, so I 
don't see why it would be a big issue.



Maybe we can get away with *ignoring* such files, perhaps after emitting
a warning.


I'd prefer an an error (or base64 encoding) rather than just skipping a 
file.  The latter sounds scary.


Regards,
--
-David
da...@pgmasters.net

Re: making the backend's json parser work in frontend code

[Mark Dilger]

> On Jan 24, 2020, at 8:36 AM, David Steele  wrote:
> 
>> I don't entirely follow why we're discussing this at all, if the
>> requirement is backing up a PG data directory.  There are not, and
>> are never likely to be, any legitimate files with non-ASCII names
>> in that context.  Why can't we just skip any such files?
> 
> It's not uncommon in my experience for users to drop odd files into PGDATA 
> (usually versioned copies of postgresql.conf, etc.), but I agree that it 
> should be discouraged.  Even so, I don't recall ever seeing any non-ASCII 
> filenames.
> 
> Skipping files sounds scary, I'd prefer an error or a warning (and then 
> base64 encode the filename).

I tend to agree with Tom.  We know that postgres doesn’t write any such files 
now, and if we ever decided to change that, we could change this, too.  So for 
now, we can assume any such files are not ours.  Either the user manually 
scribbled in this directory, or had a tool (antivirus checksum file, vim 
.WHATEVER.swp file, etc) that did so.  Raising an error would break any 
automated backup process that hit this issue, and base64 encoding the file name 
and backing up the file contents could grab data that the user would not 
reasonably expect in the backup.  But this argument applies equally well to 
such files regardless of filename encoding.  It would be odd to back them up 
when they happen to be valid UTF-8/ASCII/whatever, but not do so when they are 
not valid.  I would expect, therefore, that we only back up files which match 
our expected file name pattern and ignore (perhaps with a warning) everything 
else.

Quoting from Robert’s email about why we want a backup manifest seems to 
support this idea, at least as I see it:

> So, let's suppose we invent a backup manifest. What should it contain?
> I imagine that it would consist of a list of files, and the lengths of
> those files, and a checksum for each file. I think you should have a
> choice of what kind of checksums to use, because algorithms that used
> to seem like good choices (e.g. MD5) no longer do; this trend can
> probably be expected to continue. Even if we initially support only
> one kind of checksum -- presumably SHA-something since we have code
> for that already for SCRAM -- I think that it would also be a good
> idea to allow for future changes. And maybe it's best to just allow a
> choice of SHA-224, SHA-256, SHA-384, and SHA-512 right out of the
> gate, so that we can avoid bikeshedding over which one is secure
> enough. I guess we'll still have to argue about the default. I also
> think that it should be possible to build a manifest with no
> checksums, so that one need not pay the overhead of computing
> checksums if one does not wish. Of course, such a manifest is of much
> less utility for checking backup integrity, but you can still check
> that you've got the right files, which is noticeably better than
> nothing.  The manifest should probably also contain a checksum of its
> own contents so that the integrity of the manifest itself can be
> verified. And maybe a few other bits of metadata, but I'm not sure
> exactly what.  Ideas?
> 
> 
> 
> Once we invent the concept of a backup manifest, what do we need to do
> with them? I think we'd want three things initially:
> 
> 
> 
> (1) When taking a backup, have the option (perhaps enabled by default)
> to include a backup manifest.
> (2) Given an existing backup that has not got a manifest, construct one.
> (3) Cross-check a manifest against a backup and complain about extra
> files, missing files, size differences, or checksum mismatches.


Nothing in there sounds to me like it needs to include random cruft.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Allow to_date() and to_timestamp() to accept localized names

[Juan José Santamaría Flecha]

On Fri, Jan 24, 2020 at 5:46 PM Tom Lane  wrote:

> Peter Eisentraut  writes:
> > Looking through the patch quickly, if you want to get Unicode-fancy,
> > doing a case-insensitive comparison by running lower-case on both
> > strings is also wrong in corner cases.  All the Greek month names end in
> > sigma, so I suspect that this patch might not work correctly in such
> cases.
>
> Hm.  That's basically what citext does, and I don't recall hearing
> complaints about that.  What other definition of "case insensitive"
> would you suggest?
>
>
To illustrate the issue, it does not work as expected:

postgres=# select lower(to_char(now(),'TMMONTH'));
   lower

 ιανουάριοσ
(1 row)

postgres=# select to_char(now(),'TMmonth');
  to_char

 ιανουάριος
(1 row)

Regards,

Juan José Santamaría Flecha

any work item suggestion for newbie?

[Xiang Xiao]

hi there

I am a database developer with 10+ years industry experience. I want to
make contribution to Postgres and look for some work items to start with.
Looking at the TODO list, I am not sure what to start with? Any suggestions?

Thanks,
Xiang

Re: Duplicate Workers entries in some EXPLAIN plans

[Tom Lane]

Maciek Sakrejda  writes:
> Great, thank you. I noticed in the CF patch tester app, the build
> fails on Windows [1]. Investigating, I realized I had failed to fully
> strip volatile EXPLAIN info (namely buffers) in TEXT mode due to a
> bad regexp_replace.

You haven't gone nearly far enough in that direction.  The proposed
output still relies on the assumption that the session will always
get as many workers as it asks for, which it will not.  For previous
bitter experience in this department, see for instance commits 4ea03f3f4
and 13e8b2ee8.

TBH I am not sure that the proposed regression tests for this change
can be committed at all.  Which is a bit of a problem perhaps, but
then again we don't have terribly good coverage for the existing code
either, for the same reason.

regards, tom lane

Re: making the backend's json parser work in frontend code

[Alvaro Herrera]

On 2020-Jan-24, David Steele wrote:

> On 1/24/20 10:00 AM, Alvaro Herrera wrote:

> > Maybe we can get away with *ignoring* such files, perhaps after emitting
> > a warning.
> 
> I'd prefer an an error (or base64 encoding) rather than just skipping a
> file.  The latter sounds scary.

Well, if the file is "invalid" then evidently Postgres cannot possibly
care about it, so why would it care if it's missing from the backup?

I prefer the encoding scheme myself.  I don't see the point of the
error.

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

Re: making the backend's json parser work in frontend code

[Tom Lane]

Alvaro Herrera  writes:
> I prefer the encoding scheme myself.  I don't see the point of the
> error.

Yeah, if we don't want to skip such files, then storing them using
a base64-encoded name (with a different key than regular names)
seems plausible.  But I don't really see why we'd go to that much
trouble, nor why we'd think it's likely that tools would correctly
handle a case that is going to have 0.00% usage in the field.

regards, tom lane

Re: making the backend's json parser work in frontend code

[Alvaro Herrera]

On 2020-Jan-24, Mark Dilger wrote:

> I would expect, therefore, that we only back up files which match our
> expected file name pattern and ignore (perhaps with a warning)
> everything else.

That risks missing files placed in the datadir by extensions; see
discussion about pg_checksums using a whitelist[1], which does not
translate directly to this problem, because omitting to checksum a file
is not the same as failing to copy a file into the backups.
(Essentially, the backups would be incomplete.)

[1] https://postgr.es/m/20181019171747.4uithw2sjkt6m...@alap3.anarazel.de

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

Re: making the backend's json parser work in frontend code

[Tom Lane]

Alvaro Herrera  writes:
> On 2020-Jan-24, Mark Dilger wrote:
>> I would expect, therefore, that we only back up files which match our
>> expected file name pattern and ignore (perhaps with a warning)
>> everything else.

> That risks missing files placed in the datadir by extensions;

I agree that assuming we know everything that will appear in the
data directory is a pretty unsafe assumption.  But no rational
extension is going to use a non-ASCII file name, either, if only
because it can't predict what the filesystem encoding will be.

regards, tom lane

Re: making the backend's json parser work in frontend code

[Robert Haas]

On Fri, Jan 24, 2020 at 9:48 AM Tom Lane  wrote:
> Alvaro Herrera  writes:
> > I prefer the encoding scheme myself.  I don't see the point of the
> > error.
>
> Yeah, if we don't want to skip such files, then storing them using
> a base64-encoded name (with a different key than regular names)
> seems plausible.  But I don't really see why we'd go to that much
> trouble, nor why we'd think it's likely that tools would correctly
> handle a case that is going to have 0.00% usage in the field.

I mean, I gave a not-totally-unrealistic example of how this could
happen upthread. I agree it's going to be rare, but it's not usually
OK to decide that if a user does something a little unusual,
not-obviously-related features subtly break.

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

Re: Allow to_date() and to_timestamp() to accept localized names

[Peter Eisentraut]

On 2020-01-24 18:25, Juan José Santamaría Flecha wrote:

To illustrate the issue, it does not work as expected:

postgres=# select lower(to_char(now(),'TMMONTH'));
    lower

  ιανουάριοσ
(1 row)

postgres=# select to_char(now(),'TMmonth');
   to_char

  ιανουάριος
(1 row)


Well, this is interesting, because on macOS and Debian stable I get

postgres=# select to_char(now(),'TMmonth');
  to_char

 ιανουαρίου
(1 row)

which is the genitive of ιανουάριος.  You use the genitive form for a 
date (24th of January) but the nominative otherwise.  But the reverse 
mapping can only take one of these forms.  So here


select to_date('Ιανουαριος', 'TMMonth');

fails, which is bad.

In the glibc locale data sources they have both forms listed, but 
apparently the API were are using only accepts one of them.


(I don't know any Greek, I'm just extrapolating from Wikipedia and 
locale data.)


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

Re: making the backend's json parser work in frontend code

[Alvaro Herrera]

On 2020-Jan-24, Tom Lane wrote:

> Alvaro Herrera  writes:
> > On 2020-Jan-24, Mark Dilger wrote:
> >> I would expect, therefore, that we only back up files which match our
> >> expected file name pattern and ignore (perhaps with a warning)
> >> everything else.
> 
> > That risks missing files placed in the datadir by extensions;
> 
> I agree that assuming we know everything that will appear in the
> data directory is a pretty unsafe assumption.  But no rational
> extension is going to use a non-ASCII file name, either, if only
> because it can't predict what the filesystem encoding will be.

I see two different arguments. One is about the file encoding. Those
files are rare and would be placed by the user manually.  We can fix
that by encoding the name.  We can have a debug mode that encodes all
names that way, just to ensure the tools are prepared for it.

The other is Mark's point about "expected file pattern", which seems a
slippery slope to me.  If the pattern is /^[a-zA-Z0-9_.]*$/ then I'm
okay with it (maybe add a few other punctuation chars); as you say no
sane extension would use names much weirder than that.  But we should
not be stricter, such as counting the number of periods/underscores
allowed or where are alpha chars expected (except maybe disallow period
at start of filename), or anything too specific like that.

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

Re: making the backend's json parser work in frontend code

[Mark Dilger]

> On Jan 24, 2020, at 10:03 AM, Alvaro Herrera  wrote:
> 
> The other is Mark's point about "expected file pattern", which seems a
> slippery slope to me.  If the pattern is /^[a-zA-Z0-9_.]*$/ then I'm
> okay with it (maybe add a few other punctuation chars); as you say no
> sane extension would use names much weirder than that.  But we should
> not be stricter, such as counting the number of periods/underscores
> allowed or where are alpha chars expected (except maybe disallow period
> at start of filename), or anything too specific like that.

What bothered me about skipping files based only on encoding is that it creates 
hard to anticipate bugs.  If extensions embed something, like a customer name, 
into a filename, and that something is usually ASCII, or usually valid UTF-8, 
and gets backed up, but then some day they embed something that is not 
ASCII/UTF-8, then it does not get backed up, and maybe nobody notices until 
they actually *need* the backup, and it’s too late.

We either need to be really strict about what gets backed up, so that nobody 
gets a false sense of security about what gets included in that list, or we 
need to be completely permissive, which would include files named in arbitrary 
encodings.  I don’t see how it does anybody any favors to make the system 
appear to back up everything until you hit this unanticipated case and then it 
fails.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: making the backend's json parser work in frontend code

[Peter Eisentraut]

On 2020-01-24 18:56, Robert Haas wrote:

On Fri, Jan 24, 2020 at 9:48 AM Tom Lane  wrote:

Alvaro Herrera  writes:

I prefer the encoding scheme myself.  I don't see the point of the
error.


Yeah, if we don't want to skip such files, then storing them using
a base64-encoded name (with a different key than regular names)
seems plausible.  But I don't really see why we'd go to that much
trouble, nor why we'd think it's likely that tools would correctly
handle a case that is going to have 0.00% usage in the field.


I mean, I gave a not-totally-unrealistic example of how this could
happen upthread. I agree it's going to be rare, but it's not usually
OK to decide that if a user does something a little unusual,
not-obviously-related features subtly break.


Another example might be log files under pg_log with localized weekday 
or month names.  (Maybe we're not planning to back up log files, but the 
routines that deal with file names should probably be prepared to at 
least look at the name and decide that they don't care about it rather 
than freaking out right away.)


I'm not fond of the base64 idea btw., because it seems to sort of 
penalize using non-ASCII characters by making the result completely not 
human readable.  Something along the lines of MIME would be better in 
that way.  There are existing solutions to storing data with metadata 
around it.


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

Re: fix for BUG #3720: wrong results at using ltree

[Tomas Vondra]

Hi Nikita,

This patch seems inactive / stuck in "waiting on author" since November.
It's marked as bugfix, so it'd be good to get it committed instead of
just punting it to the next CF.

I did a quick review, and I came mostly with the same two complaints as
Alexander ...

On Wed, Jul 17, 2019 at 09:33:46PM +0300, Alexander Korotkov wrote:

Hi Nikita,

On Tue, Jul 16, 2019 at 6:52 PM Nikita Glukhov  wrote:

I looked at "ltree syntax improvement" patch and found two more very
old bugs in ltree/lquery (fixes are attached):


Thank you for the fixes.  I've couple notes on them.

0001-Fix-max-size-checking-for-ltree-and-lquery.patch

+#define LTREE_MAX_LEVELS Min(PG_UINT16_MAX, MaxAllocSize / sizeof(nodeitem))
+#define LQUERY_MAX_LEVELS Min(PG_UINT16_MAX, MaxAllocSize / ITEMSIZE)

Looks over caution.  PG_UINT16_MAX is not even close to MaxAllocSize /
sizeof(nodeitem) or MaxAllocSize / ITEMSIZE.



Yeah, I'm also puzzled by the usage of PG_UINT16_MAX here. It's so much
lower than the other values we could jut use the constant directly, but
let's say the structs could grow from the ~16B to chnge this.

The main question is why we need PG_UINT16_MAX at all? It kinda implies
we need to squish the value into a 2B counter or something, but is that
actually true? I don't see anything like that in ltree_io.c.

So it seems more like an arbitrary value considered "sane" - which is
fine, but then a comment saying so would be nice, and we could pick a
value that is "nicer" for humans. Or just use value computed from the
MaxAllocSize limit, without the Min().


0002-Fix-successive-lquery-ops.patch

diff --git a/contrib/ltree/lquery_op.c b/contrib/ltree/lquery_op.c
index 62172d5..d4f4941 100644
--- a/contrib/ltree/lquery_op.c
+++ b/contrib/ltree/lquery_op.c
@@ -255,8 +255,8 @@ checkCond(lquery_level *curq, int query_numlevel,
ltree_level *curt, int tree_nu
 }
 else
 {
- low_pos = cur_tpos + curq->low;
- high_pos = cur_tpos + curq->high;
+ low_pos = Min(low_pos + curq->low, PG_UINT16_MAX);
+ high_pos = Min(high_pos + curq->high, PG_UINT16_MAX);
 if (ptr && ptr->q)
 {
 ptr->nq++;
@@ -282,8 +282,8 @@ checkCond(lquery_level *curq, int query_numlevel,
ltree_level *curt, int tree_nu
 }
 else
 {
- low_pos = cur_tpos + curq->low;
- high_pos = cur_tpos + curq->high;
+ low_pos = Min(low_pos + curq->low, PG_UINT16_MAX);
+ high_pos = Min(high_pos + curq->high, PG_UINT16_MAX);
 }

 curq = LQL_NEXT(curq);

I'm not sure what do these checks do.  Code around is uncommented and
puzzled.  But could we guarantee the same invariant on the stage of
ltree/lquery parsing?



Unfortunately, the current code is somewhat undercommented :-(

Anyway, I don't quite understand why we need these caps. It kinda seems
like a band-aid for potential overflow.

Why should it be OK for the values to even get past the maximum, with
sane input data? And isn't there a better upper limit (e.g. based on
how much space we actually allocated)?


regards

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

Re: making the backend's json parser work in frontend code

[Robert Haas]

On Thu, Jan 23, 2020 at 1:05 PM Mark Dilger
 wrote:
> Ok, I finished merging with Robert’s patches.  The attached follow his 
> numbering, with my patches intended to by applied after his.

I think it'd be a good idea to move the pg_wchar.h stuff into a new
thread. This thread is getting a bit complicated, because we've got
(1) the patches need to do $SUBJECT plus (2) additional patches that
clean up the multibyte stuff more plus (3) discussion of issues that
pertain to the backup manifest thread. To my knowledge, $SUBJECT
doesn't strictly require the pg_wchar.h changes, so I suggest we try
to segregate those.

> I tried not to change his work too much, but I did a bit of refactoring in 
> 0010, as explained in the commit comment.

Hmm, I generally prefer to avoid these kinds of macro tricks because I
think they can be confusing to the reader. It's worth it in a case
like equalfuncs.c where so much boilerplate code is saved that the
gain in readability more than makes up for having to go check what the
macros do -- but I don't feel that's the case here. There aren't
*that* many call sites, and I think the code will be easier to
understand without "return" statements concealed within macros...

> I ran some benchmarks for json parsing in the backend both before and after 
> these patches, with very slight changes in runtime.

Cool, thanks.

Since 0001-0003 have been reviewed by multiple people and nobody's
objected, I have committed those. But I made a hash of it: the first
one, I failed to credit any reviewers, or include a Discussion link,
and I just realized that I should have listed Alvaro's name as a
reviewer also. Sorry about that.

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

Re: making the backend's json parser work in frontend code

[Alvaro Herrera]

On 2020-Jan-24, Peter Eisentraut wrote:

> I'm not fond of the base64 idea btw., because it seems to sort of penalize
> using non-ASCII characters by making the result completely not human
> readable.  Something along the lines of MIME would be better in that way.
> There are existing solutions to storing data with metadata around it.

You mean quoted-printable?  That works for me.

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

Re: error context for vacuum to include block number

[Justin Pryzby]

Thanks for reviewing

On Wed, Jan 22, 2020 at 05:37:06PM +0900, Masahiko Sawada wrote:
> I'm not sure it's worth to have patches separately but I could apply

The later patches expanded on the initial scope, and to my understanding the
1st callback is desirable but the others are maybe still at question.

> 1. * The comment should be updated as we use both relname and
> relnamespace for ereporting.
> 
> * We can leave both blkno and stage that are used only for error
> context reporting put both relname and relnamespace to top of
> LVRelStats.

Updated in the 0005 - still not sure if that patch will be desirable, though.
Also, I realized that it's we cannot use vacrelstats->blkno instead of local
blkno variable, since vacrelstats->blkno is used simultaneously by scan heap
and vacuum heap).

> * The 'stage' is missing to support index cleanup.

But the callback isn't used during index cleanup ?

> * It seems to me strange that only initialization of latestRemovedXid
> is done after error callback initialization.

Yes, that was silly - I thought it was just an artifact of diff's inability to
express rearranged code any better.

> * Maybe we can initialize relname and relnamespace in heap_vacuum_rel
> rather than in lazy_scan_heap. BTW variables of vacrelstats are
> initialized different places: some of them in heap_vacuum_rel and
> others in lazy_scan_heap. I think we can gather those that can be
> initialized at that time to heap_vacuum_rel.

I think that's already true ?  But topic for a separate patch, if not.

> 3. Maybe we can do like:

done

> 4. These comments can be merged like:

done

> 5. Why do we need to initialize blkno in spite of not using?

removed

> 6.
> +   cbarg->blkno, cbarg->relnamespace, cbarg->relname);
> * 'vacrelstats' would be a better name than 'cbarg'.

Really?  I'd prefer to avoid repeating long variable three times:

vacrelstats->blkno, vacrelstats->relnamespace, 
vacrelstats->relname);

> * In index vacuum, how about "while vacuuming index \"%s.%s\""?

Yes.  I'm still unclear if this is useful without a block number, or why it
wouldn't be better to write DEBUG1 log with index name before vacuuming each.

Justin
>From 6332127178e29967dfeb12577eb9a61e813a33a8 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Thu, 12 Dec 2019 20:54:37 -0600
Subject: [PATCH v13 1/5] vacuum errcontext to show block being processed

As requested here.
https://www.postgresql.org/message-id/20190807235154.erbmr4o4bo6vgnjv%40alap3.anarazel.de
---
 src/backend/access/heap/vacuumlazy.c | 41 ++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index b331f4c..822fa3d 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -287,8 +287,12 @@ typedef struct LVRelStats
 	int			num_index_scans;
 	TransactionId latestRemovedXid;
 	bool		lock_waiter_detected;
-} LVRelStats;
 
+	/* Used by the error callback */
+	char		*relname;
+	char 		*relnamespace;
+	BlockNumber blkno;
+} LVRelStats;
 
 /* A few variables that don't seem worth passing around as parameters */
 static int	elevel = -1;
@@ -358,6 +362,7 @@ static void end_parallel_vacuum(Relation *Irel, IndexBulkDeleteResult **stats,
 LVParallelState *lps, int nindexes);
 static LVSharedIndStats *get_indstats(LVShared *lvshared, int n);
 static bool skip_parallel_vacuum_index(Relation indrel, LVShared *lvshared);
+static void vacuum_error_callback(void *arg);
 
 
 /*
@@ -721,6 +726,7 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
 		PROGRESS_VACUUM_MAX_DEAD_TUPLES
 	};
 	int64		initprog_val[3];
+	ErrorContextCallback errcallback;
 
 	pg_rusage_init(&ru0);
 
@@ -867,6 +873,16 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
 	else
 		skipping_blocks = false;
 
+	/* Setup error traceback support for ereport() */
+	vacrelstats->relnamespace = get_namespace_name(RelationGetNamespace(onerel));
+	vacrelstats->relname = relname;
+	vacrelstats->blkno = InvalidBlockNumber; /* Not known yet */
+
+	errcallback.callback = vacuum_error_callback;
+	errcallback.arg = (void *) vacrelstats;
+	errcallback.previous = error_context_stack;
+	error_context_stack = &errcallback;
+
 	for (blkno = 0; blkno < nblocks; blkno++)
 	{
 		Buffer		buf;
@@ -888,6 +904,8 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
 #define FORCE_CHECK_PAGE() \
 		(blkno == nblocks - 1 && should_attempt_truncation(params, vacrelstats))
 
+		vacrelstats->blkno = blkno;
+
 		pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, blkno);
 
 		if (blkno == next_unskippable_block)
@@ -984,13 +1002,18 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
 vmbuffer = InvalidBuffer;
 			}
 
+			/* Pop the error context stack */
+			error_context_stack = errcallback.previous;
+
 			/* 

Re: [Proposal] Global temporary tables

[Pavel Stehule]

pá 24. 1. 2020 v 14:17 odesílatel Konstantin Knizhnik <
k.knizh...@postgrespro.ru> napsal:

>
>
> On 24.01.2020 15:15, Pavel Stehule wrote:
>
> You will see a effect of DDL in current session (where you did the
> change), all other sessions should to live without any any change do
> reconnect or to RESET connect
>
> Why? I found this requirement quit unnatural and contradicting to the
> behavior of normal tables.
> Actually one of motivation for adding global tempo tables to Postgres is
> to provide compatibility with Oracle.
> Although I know that Oracle design decisions were never considered as
> axioms by Postgres community,
> but ni case of GTT design I think that we should take in account Oracle
> approach.
> And GTT in Oracle behaves exactly as in my implementation:
>
> https://www.oracletutorial.com/oracle-basics/oracle-global-temporary-table/
>
> It is not clear from this documentation whether index created for GTT in
> one session can be used in another session which already has some data in
> this GTT.
> But I did experiment with install Oracle server and  can confirm that
> actually works in this way.
>
> So I do not understand why do we need to complicate our GTT implementation
> in order to prohibit useful functionality and introduce inconsistency
> between behavior of normal and global temp tables.
>
>
>
> I don't like 2 - when I do index on global temp table, I don't would to
> wait on indexing on all other sessions. These operations should be
> maximally independent.
>
>
> Nobody suggest to wait building index in all sessions.
> Indexes will be constructed on demand when session access this table.
> If session will no access this table at all, then index will never be
> constructed.
>

> Once again: logic of dealing with indexes in GTT is very simple.
> For normal tables, indexes are initialized at the tame when them are
> created.
> For GTT it is not true. We have to initialize index on demand when it is
> accessed first time in session.
>
> So it has to be handled in any way.
> The question is only whether we should allow creation of index for table
> already populated with some data?
> Actually doesn't require some additional efforts. We can use existed
> build_index function which initialize index and populates it with data.
> So the solution proposed for me is most natural, convenient and simplest
> solution at the same time. And compatible with Oracle.
>

I cannot to evaluate your proposal, and I am sure, so you know more about
this code.

There is a question if we can allow to build local temp index on global
temp table. It is different situation. When I work with global properties
personally I prefer total asynchronous implementation of any DDL operations
for other than current session. When it is true, then I have not any
objection. For me, good enough design of any DDL can be based on catalog
change without forcing to living tables.

I see following disadvantage of your proposal. See scenario

1. I have two sessions

A - small GTT with active owner
B - big GTT with some active application.

session A will do new index - it is fast, but if creating index is forced
on B on demand (when B was touched), then this operation have to wait after
index will be created.

So I afraid build a index on other sessions on GTT when GTT tables in other
sessions will not be empty.

Regards

Pavel



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

Re: making the backend's json parser work in frontend code

[Mark Dilger]

> On Jan 24, 2020, at 10:43 AM, Robert Haas  wrote:
> 
> Since 0001-0003 have been reviewed by multiple people and nobody's
> objected, I have committed those.

I think 0004-0005 have been reviewed and accepted by both me and Andrew, if I 
understood him correctly:

> I've reviewed these patches and Robert's, and they seem basically good to me.

Certainly, nothing in those two patches caused me any concern.  I’m going to 
modify my patches as you suggested, get rid of the INSIST macro, and move the 
pg_wchar changes to their own thread.  None of that should require changes in 
your 0004 or 0005.  It won’t bother me if you commit those two.  Andrew?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)

[Andres Freund]

Hi,

Currently pg_stat_bgwriter.buffers_backend is pretty useless to gauge
whether backends are doing writes they shouldn't do. That's because it
counts things that are either unavoidably or unlikely doable by other
parts of the system (checkpointer, bgwriter).

In particular extending the file can not currently be done by any
another type of process, yet is counted. When using a buffer access
strategy it is also very likely that writes have to be done by the
'dirtying' backend itself, as the buffer will be reused soon after (when
not previously in s_b that is).

Additionally pg_stat_bgwriter.buffers_backend also counts writes done by
autovacuum et al.


I think it'd make sense to at least split buffers_backend into
buffers_backend_extend,
buffers_backend_write,
buffers_backend_write_strat

but it could also be worthwhile to expand it into
buffers_backend_extend,
buffers_{backend,checkpoint,bgwriter,autovacuum}_write
buffers_{backend,autovacuum}_write_stat

Possibly by internally, in contrast to SQL level, having just counter
arrays indexed by backend types.


It's also noteworthy that buffers_backend is accounted in an absurd
manner. One might think that writes are accounted from backend -> shared
memory or such. But instead it works like this:

1) backend flushes buffer in bufmgr.c, accounts for backend *write time*
2) mdwrite writes and  registers a sync request, which forwards the sync 
request to checkpointer
3) ForwardSyncRequest(), when not called by bgwriter, increments 
CheckpointerShmem->num_backend_writes
4) checkpointer, whenever doing AbsorbSyncRequests(), moves
   CheckpointerShmem->num_backend_writes to
   BgWriterStats.m_buf_written_backend (local memory!)
5) Occasionally it calls pgstat_send_bgwriter(), which sends the data to
   pgstat (which bgwriter also does)
6) Which then updates the shared memory used by the display functions

Worthwhile to note that backend buffer read/write *time* is accounted
differently. That's done via pgstat_send_tabstat().


I think there's very little excuse for the indirection via checkpointer,
besides architectually being weird, it actually requires that we
continue to wake up checkpointer over and over instead of optimizing how
and when we submit fsync requests.

As far as I can tell we're also simply not accounting at all for writes
done outside of shared buffers. All writes done directly through
smgrwrite()/extend() aren't accounted anywhere as far as I can tell.


I think we also count things as writes that aren't writes: mdtruncate()
is AFAICT counted as one backend write for each segment. Which seems
weird to me.


Lastly, I don't understand what the point of sending fixed size stats,
like the stuff underlying pg_stat_bgwriter, through pgstats IPC. While
I don't like it's architecture, we obviously need something like pgstat
to handle variable amounts of stats (database, table level etc
stats). But that doesn't at all apply to these types of global stats.

Greetings,

Andres Freund

[PATCH] /src/backend/access/transam/xlog.c, tiny improvements

[Ranier Vilela]

Hi,
There are 3 tiny improvements to xlog.c code:

1. At function StartupXLOG (line 6370), the second test if
(ArchiveRecoveryRequested) is redundant and can secure removed.
2. At function StartupXLOG (line 7254) the var switchedTLI already been
tested before and the second test can secure removed.
3. At function KeepLogSeg (line 9357)  the test if (slotSegNo <= 0), the
var slotSegNo is uint64 and not can be < 0.

As it is a critical file, even though small, these improvements, I believe
are welcome, because they improve readability.

regards,
Ranier Vilela


xlog.patch
Description: Binary data

Re: BUG #16109: Postgres planning time is high across version (Expose buffer usage during planning in EXPLAIN)

[Julien Rouhaud]

On Fri, Jan 24, 2020 at 6:55 AM Justin Pryzby  wrote:
>
> On Wed, Nov 13, 2019 at 11:39:04AM +0100, Julien Rouhaud wrote:
> > (moved to -hackers)
> >
> > On Tue, Nov 12, 2019 at 9:55 PM Andres Freund  wrote:
> > >
> > > This last point is more oriented towards other PG developers: I wonder
> > > if we ought to display buffer statistics for plan time, for EXPLAIN
> > > (BUFFERS). That'd surely make it easier to discern cases where we
> > > e.g. access the index and scan a lot of the index from cases where we
> > > hit some CPU time issue. We should easily be able to get that data, I
> > > think, we already maintain it, we'd just need to compute the diff
> > > between pgBufferUsage before / after planning.
> >
> > That would be quite interesting to have.  I attach as a reference a
> > quick POC patch to implement it:
>
> +1
>
> +   result.shared_blks_hit = stop->shared_blks_hit - 
> start->shared_blks_hit;
> +   result.shared_blks_read = stop->shared_blks_read - 
> start->shared_blks_read;
> +   result.shared_blks_dirtied = stop->shared_blks_dirtied -
> +   start->shared_blks_dirtied;
> [...]
>
> I think it would be more readable and maintainable using a macro:
>
> #define CALC_BUFF_DIFF(x) result.##x = stop->##x - start->##x
> CALC_BUFF_DIFF(shared_blks_hit);
> CALC_BUFF_DIFF(shared_blks_read);
> CALC_BUFF_DIFF(shared_blks_dirtied);
> ...
> #undefine CALC_BUFF_DIFF

Good idea.  Note that you can't use preprocessor concatenation to
generate something else than a token or a number, so the ## can just
be removed here.


show_planning_buffers-v2.diff
Description: Binary data

Re: Add MAIN_RELATION_CLEANUP and SECONDARY_RELATION_CLEANUP options to VACUUM

[Bossart, Nathan]

On 1/21/20, 1:39 PM, "Vik Fearing"  wrote:
> On 21/01/2020 22:21, Bossart, Nathan wrote:
>> I've attached a patch for a couple of new options for VACUUM:
>> MAIN_RELATION_CLEANUP and SECONDARY_RELATION_CLEANUP.  The motive
>> behind these options is to allow table owners to easily vacuum only
>> the TOAST table or only the main relation.  This is especially useful
>> for TOAST tables since roles do not have access to the pg_toast schema
>> by default and some users may find it difficult to discover the name
>> of a relation's TOAST table.
>
>
> Could you explain why one would want to do this?  Autovacuum will
> already deal with the tables separately as needed, but I don't see when
> a manual vacuum would want to make this distinction.

The main use case I'm targeting is when the level of bloat or
transaction ages of a relation and its TOAST table have significantly
diverged.  In these scenarios, it could be beneficial to be able to
vacuum just one or the other, especially if the tables are large.

Nathan

Re: [UNVERIFIED SENDER] Re: Add MAIN_RELATION_CLEANUP and SECONDARY_RELATION_CLEANUP options to VACUUM

[Bossart, Nathan]

On 1/24/20, 1:32 PM, "Bossart, Nathan"  wrote:
>>> I chose to disallow disabling both *_RELATION_CLEANUP options
>>> together, as this would essentially cause the VACUUM command to take
>>> no action.
>>
>> My first reaction is why?  Agreed that it is a bit crazy to combine
>> both options, but if you add the argument related to more relation
>> types like toast..
>
> Yes, I suppose we have the same problem if you disable
> MAIN_RELATION_CLEANUP and the relation has no TOAST table.  In any
> case, allowing both options to be disabled shouldn't hurt anything.

I've been thinking further in this area, and I'm wondering if it also
makes sense to remove the restriction on ANALYZE with
MAIN_RELATION_CLEANUP disabled.  A command like

VACUUM (ANALYZE, MAIN_RELATION_CLEANUP FALSE) test;

could be interpreted as meaning we should vacuum the TOAST table and
analyze the main relation.  Since the word "cleanup" is present in the
option name, this might not be too confusing.

Nathan

Re: [HACKERS] kqueue

[Thomas Munro]

On Thu, Jan 23, 2020 at 9:38 AM Rui DeSousa  wrote:
> On Jan 22, 2020, at 2:19 PM, Tom Lane  wrote:
>> It's certainly possible that to see any benefit you need stress
>> levels above what I can manage on the small box I've got these
>> OSes on.  Still, it'd be nice if a performance patch could show
>> some improved performance, before we take any portability risks
>> for it.

You might need more than one CPU socket, or at least lots more cores
so that you can create enough contention.  That was needed to see the
regression caused by commit ac1d794 on Linux[1].

> Here is two charts comparing a patched and unpatched system.
> These systems are very large and have just shy of thousand
> connections each with averages of 20 to 30 active queries concurrently
> running at times including hundreds if not thousand of queries hitting
> the database in rapid succession.  The effect is the unpatched system
> generates a lot of system load just handling idle connections where as
> the patched version is not impacted by idle sessions or sessions that
> have already received data.

Thanks.  I can reproduce something like this on an Azure 72-vCPU
system, using pgbench -S -c800 -j32.  The point of those settings is
to have many backends, but they're all alternating between work and
sleep.  That creates a stream of poll() syscalls, and system time goes
through the roof (all CPUs pegged, but it's ~half system).  Profiling
the kernel with dtrace, I see the most common stack (by a long way) is
in a poll-related lock, similar to a profile Rui sent me off-list from
his production system.  Patched, there is very little system time and
the TPS number goes from 539k to 781k.

[1] 
https://www.postgresql.org/message-id/flat/CAB-SwXZh44_2ybvS5Z67p_CDz%3DXFn4hNAD%3DCnMEF%2BQqkXwFrGg%40mail.gmail.com

Re: Parallel grouping sets

[Jesse Zhang]

On Thu, Jan 23, 2020 at 2:47 AM Amit Kapila  wrote:
>
> On Sun, Jan 19, 2020 at 2:23 PM Richard Guo  wrote:
> >
> > I realized that there are two patches in this thread that are
> > implemented according to different methods, which causes confusion.
> >
>
> Both the idea seems to be different.  Is the second approach [1]
> inferior for any case as compared to the first approach?  Can we keep
> both approaches for parallel grouping sets, if so how?  If not, then
> won't the code by the first approach be useless once we commit second
> approach?
>
>
> [1] - 
> https://www.postgresql.org/message-id/CAN_9JTwtTTnxhbr5AHuqVcriz3HxvPpx1JWE--DCSdJYuHrLtA%40mail.gmail.com
>

I glanced over both patches. Just the opposite, I have a hunch that v3
is always better than v5. Here's my 6-minute understanding of both.

v5 (the one with a simple partial aggregate) works by pushing a little
bit of partial aggregate onto workers, and perform grouping aggregate
above gather. This has two interesting outcomes: we can execute
unmodified partial aggregate on the workers, and execute almost
unmodified rollup aggreegate once the trans values are gathered. A
parallel plan for a query like

SELECT count(*) FROM foo GROUP BY GROUPING SETS (a), (b), (c), ();

can be

Finalize GroupAggregate
  Output: count(*)
  Group Key: a
  Group Key: b
  Group Key: c
  Group Key: ()
Gather Merge
Partial GroupAggregate
  Output: PARTIAL count(*)
  Group Key: a, b, c
Sort
  Sort Key: a, b, c
Parallel Seq Scan on foo


v3 ("the one with grouping set id") really turns the plan from a tree to
a multiplexed pipe: we can execute grouping aggregate on the workers,
but only partially. When we emit the trans values, also tag the tuple
with a group id. After gather, finalize the aggregates with a modified
grouping aggregate. Unlike a non-split grouping aggregate, the finalize
grouping aggregate does not "flow" the results from one rollup to the
next one. Instead, each group only advances on partial inputs tagged for
the group.

Finalize HashAggregate
  Output: count(*)
  Dispatched by: (GroupingSetID())
  Group Key: a
  Group Key: b
  Group Key: c
Gather
Partial GroupAggregate
  Output: PARTIAL count(*), GroupingSetID()
  Group Key: a
  Sort Key: b
  Group Key: b
  Sort Key: c
  Group Key: c
Sort
  Sort Key: a
Parallel Seq Scan on foo

Note that for the first approach to be viable, the partial aggregate
*has to* use a group key that's the union of all grouping sets. In cases
where individual columns have a low cardinality but joint cardinality is
high (say columns a, b, c each has 16 distinct values, but they are
independent, so there are 4096 distinct values on (a,b,c)), this results
in fairly high traffic through the shm tuple queue.

Cheers,
Jesse

Re: Duplicate Workers entries in some EXPLAIN plans

[Maciek Sakrejda]

Okay. Does not getting as many workers as it asks for include
sometimes getting zero, completely changing the actual output? If so,
I'll submit a new version of the patch removing all tests--I was
hoping to improve coverage, but I guess this is not the way to start.
If not, can we keep the json tests at least if we only consider the
first worker?

Re: Memory-Bounded Hash Aggregation

[Peter Geoghegan]

On Fri, Jan 24, 2020 at 5:01 PM Jeff Davis  wrote:
> Unfortunately, I'm seeing some bad behavior (at least in some cases)
> with logtape.c, where it's spending a lot of time qsorting the list of
> free blocks. Adam, did you also see this during your perf tests? It
> seems to be worst with lower work_mem settings and a large number of
> input groups (perhaps there are just too many small tapes?).

That sounds weird. Might be pathological in some sense.

I have a wild guess for you. Maybe this has something to do with the
"test for presorted input" added by commit a3f0b3d68f9. That can
perform very badly when the input is almost sorted, but has a few
tuples that are out of order towards the end. (I have called these
"banana skin tuples" in the past.)

-- 
Peter Geoghegan

Re: Duplicate Workers entries in some EXPLAIN plans

[Tom Lane]

Maciek Sakrejda  writes:
> Okay. Does not getting as many workers as it asks for include
> sometimes getting zero, completely changing the actual output?

Yup :-(.  We could possibly avoid that by running the explain
test by itself rather than in a parallel group, but I don't
especially want to add yet more non-parallelizable tests.

Meanwhile, I spent some time looking at the code, and wasn't very happy
with it.  I'm on board with the general plan of redirecting EXPLAIN
output into per-worker buffers that we eventually recombine, but I think
that this particular coding is pretty unmaintainable/unextensible.
In particular, I'm really unhappy that the code is ignoring EXPLAIN's
grouping_state stack.  That means that when it's formatting fields that
belong to the per-worker group, it's using the state-stack entry that
corresponds to the plan node's main level.  This seems to accidentally
work, but that fact depends on a number of shaky assumptions:

* Before messing with any per-worker data, we've always emitted at
least one field in the plan node's main level, so that the state-stack
entry isn't at its initial state for the level.

* Before actually emitting the shunted-aside data, we've always emitted
a "Worker Number" field in correct format within the per-worker group,
so that the formatting state is now correct for a non-initial field.

* There is no formatting difference between any non-first fields in
a level (ie the state stack entries are basically just booleans),
so that it doesn't matter how many plan-node fields we emitted before
starting the per-worker data, so long as there was at least one, nor
does transiently abusing the plan node level's stack entry like this
break the formatting of subsequent plan-node-level fields.

Now maybe we'll never implement an output format that breaks that
last assumption, and maybe we'll never rearrange the EXPLAIN code
in a way that breaks either of the first two.  But I don't like those
assumptions too much.  I also didn't like the code's assumption that
all the non-text formats interpret es->indent the same.

I also noted an actual bug, which is that the patch fails regression
testing under force_parallel_mode = regress.  This isn't really your
fault, because the issue is in this obscure and poorly-commented hack
in show_sort_info:

 * You might think we should just skip this stanza entirely when
 * es->hide_workers is true, but then we'd get no sort-method output at
 * all.  We have to make it look like worker 0's data is top-level data.
 * Currently, we only bother with that for text-format output.

Nonetheless, it's broken.

So I spent some time hacking on this and came up with the attached.
It's noticeably more verbose than your patch, but it keeps the
output-format-aware code at what seems to me to be a maintainable
arm's-length distance from the parallel-worker hacking.  TEXT is
still a special case of course :-(.

This patch just covers the code, I'm not taking any position yet
about what to do about the tests.  I did tweak the code to eliminate
the one formatting difference in select_parallel (ie put two spaces
after "Worker N:", which I think reads better anyhow), so it
passes check-world as it stands.

Thoughts?

regards, tom lane

diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index d189b8d..9e965d2 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -123,11 +123,20 @@ static void ExplainSubPlans(List *plans, List *ancestors,
 			const char *relationship, ExplainState *es);
 static void ExplainCustomChildren(CustomScanState *css,
   List *ancestors, ExplainState *es);
+static ExplainWorkersState *ExplainCreateWorkersState(int num_workers);
+static void ExplainOpenWorker(int n, ExplainState *es);
+static void ExplainCloseWorker(int n, ExplainState *es);
+static void ExplainFlushWorkersState(ExplainState *es);
 static void ExplainProperty(const char *qlabel, const char *unit,
 			const char *value, bool numeric, ExplainState *es);
+static void ExplainOpenSetAsideGroup(const char *objtype, const char *labelname,
+	 bool labeled, int depth, ExplainState *es);
+static void ExplainSaveGroup(ExplainState *es, int depth, int *state_save);
+static void ExplainRestoreGroup(ExplainState *es, int depth, int *state_save);
 static void ExplainDummyGroup(const char *objtype, const char *labelname,
 			  ExplainState *es);
 static void ExplainXMLTag(const char *tagname, int flags, ExplainState *es);
+static void ExplainIndentText(ExplainState *es);
 static void ExplainJSONLineEnding(ExplainState *es);
 static void ExplainYAMLLineStarting(ExplainState *es);
 static void escape_yaml(StringInfo buf, const char *str);
@@ -827,7 +836,7 @@ ExplainPrintJIT(ExplainState *es, int jit_flags,
 	/* for higher density, open code the text output format */
 	if (es->format == EXPLAIN_FORMAT_TEXT)
 	{
-		appendStringInfoSpaces(es->str, es->indent * 2);

Re: A rather hackish POC for alternative implementation of WITH TIES

[Ryan Lambert]

On Wed, Jan 22, 2020 at 3:06 PM Alvaro Herrera 
wrote:
> My own inclination is that Andrew's implementation, being more general
> in nature, would be the better one to have in the codebase; but we don't
> have a complete patch yet.  Can we reach some compromise such as if
> Andrew's patch is not completed then we push Surafel's?

+1

On Wed, Jan 22, 2020 at 4:35 PM Andrew Gierth 
wrote:
> I was largely holding off on doing further work hoping for some
> discussion of which way we should go. If you think my approach is worth
> pursuing (I haven't seriously tested the performance, but I'd expect it
> to be slower than Surafel's - the price you pay for flexibility) then I
> can look at it further, but figuring out the planner stuff will take
> some time.

Flexibility with more generalized code is good, though if performance is
significantly slower I would be concerned.  I quickly reviewed the patch
but haven't tested it yet.

Is it realistic to add PERCENT into this patch or would that be a future
enhancement?

Thanks,

*Ryan Lambert*

Re: Parallel leader process info in EXPLAIN

[Melanie Plageman]

So, I think from a code review perspective the code in the patches
LGTM.
As for the EXPLAIN ANALYZE tests--I don't see that many of them in
regress, so maybe that's because they aren't normally very useful.  In
this case, it would only be to protect against regressions in printing
the leader instrumentation, I think.
The problem with that is, even with all the non-deterministic info
redacted, if the leader doesn't participate (which is not guaranteed),
then its stats wouldn't be printed at all and that would cause an
incorrectly failing test case...okay I just talked myself out of the
usefulness of testing this.
So, I would move it to "ready for committer", but, since it is not
applying cleanly, I have changed the status to "waiting on author".

Re: Duplicate Workers entries in some EXPLAIN plans

[Maciek Sakrejda]

Thanks for the thorough review. I obviously missed some critical
issues. I recognized some of the other maintainability problems, but
did not have a sense of how to best avoid them, being unfamiliar with
the code.

For what it's worth, this version makes sense to me.

Re: Parallel leader process info in EXPLAIN

[Thomas Munro]

On Sat, Jan 25, 2020 at 3:39 PM Melanie Plageman
 wrote:
> So, I think from a code review perspective the code in the patches
> LGTM.
> As for the EXPLAIN ANALYZE tests--I don't see that many of them in
> regress, so maybe that's because they aren't normally very useful.  In
> this case, it would only be to protect against regressions in printing
> the leader instrumentation, I think.
> The problem with that is, even with all the non-deterministic info
> redacted, if the leader doesn't participate (which is not guaranteed),
> then its stats wouldn't be printed at all and that would cause an
> incorrectly failing test case...okay I just talked myself out of the
> usefulness of testing this.
> So, I would move it to "ready for committer", but, since it is not
> applying cleanly, I have changed the status to "waiting on author".

Hi Melanie,

Thanks for the reviews!

I think I'm going to abandon 0002 for now, because that stuff is being
refactored independently over here, so rebasing would be futile:

https://www.postgresql.org/message-id/flat/CAOtHd0AvAA8CLB9Xz0wnxu1U%3DzJCKrr1r4QwwXi_kcQsHDVU%3DQ%40mail.gmail.com

On that basis, I've set it to ready for committer (meaning 0001 only).
Thanks for the rebase.  I'll let that sit for a couple of days and see
if anything conflicting comes out of that other thread.  It's a fair
complaint that we lack tests that show the new output; I'll think
about adding one too.

Re: BufFileRead() error signalling

[Thomas Munro]

On Wed, Dec 11, 2019 at 2:07 AM Ibrar Ahmed  wrote:
> You are checking file->dirty twice, first before calling the function and 
> within the function too. Same for the Assert. For example.

True.  Thanks for the review.  Before I post a new version, any
opinions on whether to back-patch, and whether to do 0003 in master
only, or at all?

Re: Error message inconsistency

[Amit Kapila]

On Fri, Jan 24, 2020 at 9:37 PM Tom Lane  wrote:
>
> Amit Kapila  writes:
> > LGTM.  I have combined them into the single patch.  What do we think
> > about backpatching this?
>
> No objection to the patch for HEAD, but it does not seem like
> back-patch material: it is not fixing a bug.
>

Okay, I will commit this early next week (by Tuesday).

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

Re: [HACKERS] Block level parallel vacuum

[Amit Kapila]

On Fri, Jan 24, 2020 at 4:58 PM Mahendra Singh Thalor
 wrote:
>
> On Thu, 23 Jan 2020 at 15:32, Mahendra Singh Thalor  
> wrote:
> >
> > On Wed, 22 Jan 2020 at 12:48, Masahiko Sawada
> >  wrote:
> > >
> > > Attached the updated version patch.
> >
> > Thanks Sawada-san for the re-based patch.
> >
> > I reviewed and tested this patch.  Patch looks good to me.
>
> As offline, suggested by Amit Kapila, I verified vacuumdb "-P" option
> functionality with older versions(<13) and also I tested vacuumdb by
> giving "-j" option with "-P". All are working as per expectation and I
> didn't find any issue with these options.
>

I have made few modifications in the patch.

1. I think we should try to block the usage of 'full' and 'parallel'
option in the utility rather than allowing the server to return an
error.
2. It is better to handle 'P' option in getopt_long in the order of
its declaration in long_options array.
3. Added an Assert for server version while handling of parallel option.
4. Added a few sentences in the documentation.

What do you guys think of the attached?

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


v37-0001-Add-parallel-option-to-vacuumdb-command.patch
Description: Binary data