Re: [HACKERS] Runtime Partition Pruning

[Beena Emerson]

Hello David,

On Wed, Dec 27, 2017 at 8:36 AM, David Rowley
 wrote:
> Hi,
>
> Please find attached my 4th version this patch.

Thanks for the patch
>
> This is now based on v17 of Amit's faster partition pruning patch [1].
> It also now includes Beena's tests which I've done some mostly
> cosmetic changes to.
>
> I've also fixed a few bugs, one in a case where I was not properly
> handling zero matching partitions in nodeAppend.c.
>
> Another change I've made is to now perform the partition pruning at
> run-time using a new memory context that's reset each time we
> redetermine the matching partitions. This was required since we're
> calling a planner function which might not be too careful about
> pfreeing memory it allocates. A test case I was running before making
> this change ended out failing to palloc memory due to OOM.
>
> I've not done anything about reducing the cost of the Append path when
> runtime pruning is enabled. I'm still thinking over the best way to
> handle that.
>

I think you are testing without asserts

The following assert fails: src/backend/optimizer/plan/setrefs.c :
set_plan_refs: ln 921
Assert(splan->plan.qual == NIL);
Append node now has runtime partition quals.

Also since the valid subplans are set in ExecInitAppend, the queries
with Init Plans do not work. I had moved it to ExecAppend in my patch
to handle the InitPlans as well.

DROP TABLE IF EXISTS prun_test_part;
CREATE TABLE prun_test_part (sal int) PARTITION BY RANGE(sal);
CREATE TABLE prun_test_part_p1 PARTITION OF prun_test_part FOR VALUES
FROM (0) TO (100);
CREATE TABLE prun_test_part_p2 PARTITION OF prun_test_part FOR VALUES
FROM (100) TO (200);
CREATE TABLE prun_test_part_p3 PARTITION OF prun_test_part FOR VALUES
FROM (200) TO (300);
CREATE TABLE prun_test_part_p4 PARTITION OF prun_test_part FOR VALUES
FROM (300) TO (400);

INSERT INTO prun_test_part VALUES (90), (100), (110), (200), (210),
(300), (310);
=# explain (analyze, costs off, summary off, timing off) SELECT * FROM
prun_test_part WHERE sal < (SELECT sal FROM prun_test_part WHERE sal =
200);
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.


-- 

Beena Emerson

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

Re: Increasing timeout of poll_query_until for TAP tests

[Noah Misch]

On Tue, Aug 02, 2016 at 06:21:03PM -0400, Alvaro Herrera wrote:
> Michael Paquier wrote:
> > $node_master->safe_psql('postgres',
> > "INSERT INTO tab_int VALUES (generate_series(1001,2000))");
> > my $recovery_txid =
> >   $node_master->safe_psql('postgres', "SELECT txid_current()");
> > my $lsn2 =
> >   $node_master->safe_psql('postgres', "SELECT pg_current_xlog_location();");
> > What I think we had better do is reverse the calls
> > pg_current_xlog_location() and txid_current() so as we are sure that
> > the LSN we track for replay is lower than the real target LSN. The
> > same problem exists when defining the timestamp target.
> > 
> > The patch attached does that,
> 
> Why not capture both items in a single select, such as in the attached
> patch?

> -my $recovery_time = $node_master->safe_psql('postgres', "SELECT now()");
> -my $lsn3 =
> -  $node_master->safe_psql('postgres', "SELECT pg_current_xlog_location();");
> +$ret =
> +  $node_master->safe_psql('postgres', "SELECT pg_current_xlog_location(), 
> now();");
> +my ($lsn3, $recovery_time) = split /\|/, $ret;

Since now() is transaction_timestamp(), $recovery_time precedes or equals
$lsn3, and this didn't close the race.  Using clock_timestamp() here would
work, as does using separate transactions like recovery-test-fixes.patch did.
I'll shortly push a fix for this and a similar ordering problem in the
standby_5 test, which first appeared subsequent to this thread.

Re: Contributing with code

[Craig Ringer]

On 1 January 2018 at 03:27, Peter Geoghegan  wrote:

> On Sun, Dec 31, 2017 at 6:35 PM, Robert Haas 
> wrote:
> > Also, let's delete the TODO list.  People keep using it as a source of
> > project ideas, and that's bad.
>
> It would be great if the TODO list was deleted IMV.
>
> Choosing the right project is one of the hardest and most important
> parts of successfully contributing to PostgreSQL. It's usually
> essential to understand in detail why the thing that you're thinking
> of working on doesn't already exist. The TODO list seems to suggest
> almost the opposite, and as such is a trap for inexperienced hackers.
>

I don't entirely agree. It's a useful place to look for "are there other
things related to the thing I'm contemplating doing" and "has anyone tried
this? where did they get stuck?"

I'd rather rename it the "stuck, hard and abandoned projects list" ;)

For example I try to keep track of protocol-related stuff there, so that
if/when we do a real protocol revision we don't fail to consider things
that have come up and since been forgotten.

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

Re: ALTER TABLE ADD COLUMN fast default

[Andrew Dunstan]

On 12/12/2017 02:13 PM, Andrew Dunstan wrote:
>
> On 12/06/2017 11:26 AM, Andrew Dunstan wrote:
>>> In general, you need to be thinking about this in terms of making sure
>>> that it's impossible to accidentally not update code that needs to be
>>> updated.  Another example is that I noticed that some places the patch
>>> has s/CreateTupleDescCopy/CreateTupleDescCopyConstr/, evidently because
>>> the former will fail to copy the missing-attribute support data.
>>> I think that's an astonishingly bad idea.  There is basically no situation
>>> in which CreateTupleDescCopy defined in that way will ever be safe to use.
>>> The missing-attribute info is now a fundamental part of a tupdesc and it
>>> has to be copied always, just as much as e.g. atttypid.
>>>
>>> I would opine that it's a mistake to design TupleDesc as though the
>>> missing-attribute data had anything to do with default values.  It may
>>> have originated from the same place but it's a distinct thing.  Better
>>> to store it in a separate sub-structure.
>> OK, will work on all that. Thanks again.
>>
>
>
> Looking closer at this. First, there is exactly one place where the
> patch does  s/CreateTupleDescCopy/CreateTupleDescCopyConstr/.
>
> making this like atttypid suggests that it belongs right outside the
> TupleConstr structure. But then it seems to me that the change could
> well end up being a darn site more invasive. For example, should we be
> passing the number of missing values to CreateTemplateTupleDesc and
> CreateTupleDesc? I'm happy to do whatever work is required, but I want
> to have a firm understanding of the design before I spend lots of time
> cutting code. Once I understand how tupdesc.h should look I should be good.
>


Here is a new version of the patch. It separates the missing values
constructs and logic from the default values constructs and logic. The
missing values now sit alongside the default values in tupleConstr. In
some places the separation makes the logic a good bit cleaner.

Some of the logic is also reworked to remove an assumption that the
missing values structure is populated in attnum order, Also code to
support the missing stuctures is added to equalTupleDescs.

We still have that one extra place where we need to call
CreateTupleDescCopyConstr instead of CreateTupleDescCopy. I haven't seen
an easy way around that.

cheers

andrew

-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 3f02202..d5dc14a 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1150,6 +1150,18 @@
  
 
  
+  atthasmissing
+  bool
+  
+  
+This column has a value which is used where the column is entirely
+missing from the row, as happens when a column is added after the
+row is created. The actual value used is stored in the
+attmissingval  column.
+  
+ 
+
+ 
   attidentity
   char
   
@@ -1229,6 +1241,18 @@
   
  
 
+ 
+  attmissingval
+  bytea
+  
+  
+This column has a binary representation of the value used when the column
+is entirely missing from the row, as happens when the column is added after
+the row is created. The value is only used when
+atthasmissing is true.
+  
+ 
+
 

   
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 7bcf242..780bead 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -1115,26 +1115,28 @@ ALTER TABLE [ IF EXISTS ] name

 

-When a column is added with ADD COLUMN, all existing
-rows in the table are initialized with the column's default value
-(NULL if no DEFAULT clause is specified).
-If there is no DEFAULT clause, this is merely a metadata
-change and does not require any immediate update of the table's data;
-the added NULL values are supplied on readout, instead.
+ When a column is added with ADD COLUMN and a
+ non-volatile DEFAULT is specified, the default
+ is evaluated at the time of the statement and the result stored in the
+ table's metadata. That value will be used for the column for
+ all existing rows. If no DEFAULT is specified
+ NULL is used. In neither case is a rewrite of the table required.

 

-Adding a column with a DEFAULT clause or changing the type of
-an existing column will require the entire table and its indexes to be
-rewritten.  As an exception when changing the type of an existing column,
-if the USING clause does not change the column
-contents and the old type is either binary coercible to the new type or
-an unconstrained domain over the new type, a table rewrite is not needed;
-but any indexes on the affected columns must still be rebuilt.  Adding or
-removing a

Re: Foreign keys and partitioned tables

[Alvaro Herrera]

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


0001-Local-partitioned-indexes.patch.gz
Description: application/gunzip
>From b7e85e873ba77509793180e9076295fae2fd88a7 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Tue, 21 Nov 2017 15:54:14 -0300
Subject: [PATCH 2/2] [WIP] Allow foreign key triggers on partitioned tables

---
 src/backend/catalog/pg_constraint.c| 192 +
 src/backend/commands/tablecmds.c   | 105 +---
 src/backend/parser/parse_utilcmd.c |  12 --
 src/backend/utils/adt/ri_triggers.c|  50 +++-
 src/include/catalog/pg_constraint_fn.h |   2 +
 src/include/commands/tablecmds.h   |   4 +
 src/test/regress/expected/alter_table.out  | 115 -
 src/test/regress/expected/create_table.out |  10 --
 src/test/regress/expected/foreign_key.out  |  67 ++
 src/test/regress/sql/alter_table.sql   |  57 -
 src/test/regress/sql/create_table.sql  |   8 --
 src/test/regress/sql/foreign_key.sql   |  28 +
 12 files changed, 566 insertions(+), 84 deletions(-)

diff --git a/src/backend/catalog/pg_constraint.c 
b/src/backend/catalog/pg_constraint.c
index 7dee6db0eb..9dc91fb67f 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -26,6 +26,7 @@
 #include "catalog/pg_operator.h"
 #include "catalog/pg_type.h"
 #include "commands/defrem.h"
+#include "commands/tablecmds.h"
 #include "utils/array.h"
 #include "utils/builtins.h"
 #include "utils/fmgroids.h"
@@ -375,6 +376,197 @@ CreateConstraintEntry(const char *constraintName,
return conOid;
 }
 
+/*
+ * For each foreign key constraint in relation parentId, create a cloned
+ * copy of it for relationId.
+ *
+ * relationId is a partition of parentId, so we can be certain that it has
+ * the same columns with the same datatypes.  They may be in different order,
+ * though.
+ */
+void
+CloneForeignKeyConstraints(Oid parentId, Oid relationId)
+{
+   Relationpg_constraint;
+   Relationrel;
+   ScanKeyData key;
+   SysScanDesc scan;
+   TupleDesc   tupdesc;
+   HeapTuple   tuple;
+
+   /* see ATAddForeignKeyConstraint about lock level */
+   rel = heap_open(relationId, AccessExclusiveLock);
+
+   pg_constraint = heap_open(ConstraintRelationId, RowShareLock);
+   tupdesc = RelationGetDescr(pg_constraint);
+
+   ScanKeyInit(&key,
+   Anum_pg_constraint_conrelid, 
BTEqualStrategyNumber,
+   F_OIDEQ, ObjectIdGetDatum(parentId));
+   scan = systable_beginscan(pg_constraint, ConstraintRelidIndexId, true,
+ NULL, 1, &key);
+
+   while ((tuple = systable_getnext(scan)) != NULL)
+   {
+   Form_pg_constraint  constrForm = (Form_pg_constraint) 
GETSTRUCT(tuple);
+   AttrNumber  conkey[INDEX_MAX_KEYS];
+   AttrNumber  confkey[INDEX_MAX_KEYS];
+   Oid conpfeqop[INDEX_MAX_KEYS];
+   Oid conppeqop[INDEX_MAX_KEYS];
+   Oid conffeqop[INDEX_MAX_KEYS];
+   Constraint *fkconstraint;
+   Oid constrOid;
+   ObjectAddress parentAddr,
+   childAddr;
+   int nelem;
+   ArrayType  *arr;
+   Datum   datum;
+   boolisnull;
+
+   /* only foreign keys */
+   if (constrForm->contype != CONSTRAINT_FOREIGN)
+   continue;
+
+   ObjectAddressSet(parentAddr, ConstraintRelationId,
+HeapTupleGetOid(tuple));
+
+   datum = fastgetattr(tuple, Anum_pg_constraint_conkey,
+   tupdesc, &isnull);
+   if (isnull)
+   elog(ERROR, "null conkey");
+   arr = DatumGetArrayTypeP(datum);
+   nelem = ARR_DIMS(arr)[0];
+   if (ARR_NDIM(arr) != 1 ||
+   nelem < 1 ||
+   nelem > INDEX_MAX_KEYS ||
+   ARR_HASNULL(arr) ||
+   ARR_ELEMTYPE(arr) != INT2OID)
+   elog(ERROR, "conkey is not a 1-D smallint array");
+   memcpy(conkey, ARR_DATA_PTR(arr), nelem * sizeof(AttrNumber));
+
+   datum = fastgetattr(tuple, Anum_pg_constraint_confkey,
+   tupdesc, &isnull);
+   if (isnull)
+   elog(ERROR, "null confkey");
+   arr = DatumGetArrayTypeP(datum);
+   nelem = ARR_DIMS(arr)[0];
+   if (ARR_N

Foreign keys and partitioned tables

[Alvaro Herrera]

This patch enables foreign key constraints to and from partitioned
tables.

Naturally, FKs that reference a partitioned table require unique
constraints, and therefore they shares the restrictions of those: in my
proposed patch, it is only possible if the partition keys are part of
the unique constraint.  That's not explicitly checked by the FK code,
but rather just an property emergent of previous patches.

As far as I can tell, no documentation changes are needed, since AFAICS
we don't claim anywhere that FKs are not supported for partitioned
tables.

pg_dump support is not yet correct here, but otherwise this feature
should work as intended, and all tests pass for me.

I haven't gone exhaustively over things such as partitions created in
odd ways, dropped columns, match partial, etc, so bugs, holes and
non-working corner cases are still expected, but please do report any
you find.

This patch removes all the ONLY markers from queries in ri_triggers.c.
That makes the queries work for the new use case, but I haven't figured
if it breaks things for other use cases.  I suppose not, since regular
inheritance isn't supposed to allow foreign keys in the first place, but
I haven't dug any further.

Patch 0001 attached here corresponds to a squashed version of patches in
other threads; it's here just for convenience.  The patch to be reviewed
for this thread is just 0002 and corresponding functionality.

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

Re: TODO list (was Re: Contributing with code)

[Peter Geoghegan]

On Sun, Dec 31, 2017 at 10:42 AM, Tom Lane  wrote:
> If we're not going to maintain/curate it properly, I agree it's not
> worth keeping it around.  But I'd rather see somebody put some effort
> into it ...

If somebody was going to resolve to put some effort into maintaining
it to a high standard then it probably would have happened already.
The fact that it hasn't happened tells us plenty.

-- 
Peter Geoghegan

Re: Contributing with code

[Peter Geoghegan]

On Sun, Dec 31, 2017 at 6:35 PM, Robert Haas  wrote:
> Also, let's delete the TODO list.  People keep using it as a source of
> project ideas, and that's bad.

It would be great if the TODO list was deleted IMV.

Choosing the right project is one of the hardest and most important
parts of successfully contributing to PostgreSQL. It's usually
essential to understand in detail why the thing that you're thinking
of working on doesn't already exist. The TODO list seems to suggest
almost the opposite, and as such is a trap for inexperienced hackers.

-- 
Peter Geoghegan

Add default role 'pg_access_server_files'

[Stephen Frost]

Greetings,

This patch adds a new default role called 'pg_access_server_files' which
allows an administrator to GRANT to a non-superuser role the ability to
access server-side files through PostgreSQL (as the user the database is
running as).  By itself, having this role allows a non-superuser to use
server-side COPY and to use file_fdw (if installed by a superuser and
GRANT'd USAGE on it).

Further, this patch moves the privilege check for the remaining misc
file functions from explicit superuser checks to the GRANT system,
similar to what's done for pg_ls_logdir() and others.  Lastly, these
functions are changed to allow a user with the 'pg_access_server_files'
role to be able to access files outside of the PG data directory.

This follows on and continues what was recently done with the
lo_import/export functions.  There's other superuser checks to replace
with grant'able default roles, but those probably make more sense as
independent patches.  I continue to be of the opinion that it'd be nice
to have more fine-grained control over these functions to limit the
access granted, but nothing here prevents that from being done and this
at least allows some movement away from having to have roles with
superuser access.

Thanks!

Stephen
From eb8be8ffbadcc37418dc12d59c6767e073028e35 Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Sun, 31 Dec 2017 14:01:12 -0500
Subject: [PATCH] Add default role pg_access_server_files

This patch adds a new default role called 'pg_access_server_files' which
allows an administrator to GRANT to a non-superuser role the ability to
access server-side files through PostgreSQL (as the user the database is
running as).  By itself, having this role allows a non-superuser to use
server-side COPY and to use file_fdw (if installed by a superuser and
GRANT'd USAGE on it).

Further, this patch moves the privilege check for the remaining misc
file functions from explicit superuser checks to the GRANT system,
similar to what's done for pg_ls_logdir() and others.  Lastly, these
functions are changed to allow a user with the 'pg_access_server_files'
role to be able to access files outside of the PG data directory.
---
 contrib/file_fdw/file_fdw.c  | 13 -
 doc/src/sgml/file-fdw.sgml   |  9 +
 doc/src/sgml/func.sgml   | 16 
 doc/src/sgml/ref/copy.sgml   |  5 +++--
 src/backend/catalog/system_views.sql | 14 ++
 src/backend/commands/copy.c  | 16 ++--
 src/backend/utils/adt/genfile.c  | 30 ++
 src/include/catalog/pg_authid.h  |  2 ++
 8 files changed, 60 insertions(+), 45 deletions(-)

diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
index 370cc365d6..08d8d61f10 100644
--- a/contrib/file_fdw/file_fdw.c
+++ b/contrib/file_fdw/file_fdw.c
@@ -18,6 +18,7 @@
 #include "access/htup_details.h"
 #include "access/reloptions.h"
 #include "access/sysattr.h"
+#include "catalog/pg_authid.h"
 #include "catalog/pg_foreign_table.h"
 #include "commands/copy.h"
 #include "commands/defrem.h"
@@ -202,9 +203,10 @@ file_fdw_validator(PG_FUNCTION_ARGS)
 	ListCell   *cell;
 
 	/*
-	 * Only superusers are allowed to set options of a file_fdw foreign table.
-	 * This is because we don't want non-superusers to be able to control
-	 * which file gets read or which program gets executed.
+	 * Only members of the special role 'pg_access_server_files' are allowed
+	 * to set options of a file_fdw foreign table.  This is because we don't
+	 * want regular users to be able to control which file gets read or which
+	 * program gets executed.
 	 *
 	 * Putting this sort of permissions check in a validator is a bit of a
 	 * crock, but there doesn't seem to be any other place that can enforce
@@ -214,10 +216,11 @@ file_fdw_validator(PG_FUNCTION_ARGS)
 	 * program at any options level other than foreign table --- otherwise
 	 * there'd still be a security hole.
 	 */
-	if (catalog == ForeignTableRelationId && !superuser())
+	if (catalog == ForeignTableRelationId &&
+		!is_member_of_role(GetUserId(), DEFAULT_ROLE_ACCESS_SERVER_FILES))
 		ereport(ERROR,
 (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("only superuser can change options of a file_fdw foreign table")));
+ errmsg("only superuser or a member of the pg_access_server_files role can change options of a file_fdw foreign table")));
 
 	/*
 	 * Check that only options supported by file_fdw, and allowed for the
diff --git a/doc/src/sgml/file-fdw.sgml b/doc/src/sgml/file-fdw.sgml
index e2598a07da..119f55add4 100644
--- a/doc/src/sgml/file-fdw.sgml
+++ b/doc/src/sgml/file-fdw.sgml
@@ -186,10 +186,11 @@
  
 
  
-  Changing table-level options requires superuser privileges, for security
-  reasons: only a superuser should be able to control which file is read
-  or which program is run.  In principle non-superusers could be allowed to
-  change the other options, but that's not supported at present.
+  Chan

Re: TODO list (was Re: Contributing with code)

[David G. Johnston]

On Sun, Dec 31, 2017 at 11:42 AM, Tom Lane  wrote:

> Robert Haas  writes:
> > Also, let's delete the TODO list.  People keep using it as a source of
> > project ideas, and that's bad.
>
> If we're not going to maintain/curate it properly, I agree it's not
> worth keeping it around.  But I'd rather see somebody put some effort
> into it ...
>

​It probably needs three sub-sections.  Fist the raw ideas put forth by
people not capable of implementation but needing capabilities; these get
moved to one of two sections: ideas that have gotten some attention by core
that have merit but don't hav​e development interest presently; and one
like this that have gotten the some attention and that core doesn't feel
would be worth maintaining even if someone was willing to develop it.  We
already have this in practice but maybe a bit more formality would help.

I'm not seeing that having it, even if incorrect, does harm.  Those who
would develop from it should be using it as a conversation starter and even
if it is discovered that the feature is no longer desirable or applicable
that conversation will have been worthwhile to that person and probably
many others browsing these mailing lists.  I do think that all of the
commit-fest managers (and release note writers/reviewers) for the release
year could be asked/reminded to at least skim over the ToDo list at the end
of their session and see whether anything was addressed by one of the
commits that went in during their tenure and remove (or update) them.

A thread started by "hey, lets talk about this ToDo list item" is one that
reinforces the value of having such a list - and at worse should result in
that particular entry being updated in some fashion.

David J.

Re: TODO list (was Re: Contributing with code)

[Stephen Frost]

Tom, Robert, all,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Robert Haas  writes:
> > Also, let's delete the TODO list.  People keep using it as a source of
> > project ideas, and that's bad.
> 
> If we're not going to maintain/curate it properly, I agree it's not
> worth keeping it around.  But I'd rather see somebody put some effort
> into it ...

I don't see anything particularly wrong with the specific item chosen
off of the todo list- it'd be nice to be able to tell what objects exist
in a tablespace even if they're in other databases.

The todo entry even talks about why it's difficult to do and what the
expected way to go about doing it is (that is, connect to each database
that has objects in the tablespace and query it to find out what's in
the tablespace).  Craig's suggestion is an interesting alternative way
though and I'm not sure that it'd be all that bad, but it would be
limited to catalog tables.

If we'd extend the system to allow transparent usage of postgres_fdw to
access other databases which are part of the same cluster, then this
could end up being much simpler (eg: select * from
otherdatabase.pg_catalog.pg_class ...).

Thanks!

Stephen


signature.asc
Description: PGP signature

TODO list (was Re: Contributing with code)

[Tom Lane]

Robert Haas  writes:
> Also, let's delete the TODO list.  People keep using it as a source of
> project ideas, and that's bad.

If we're not going to maintain/curate it properly, I agree it's not
worth keeping it around.  But I'd rather see somebody put some effort
into it ...

regards, tom lane

Re: What does Time.MAX_VALUE actually represent?

[Gavin Flower]

Hi Bear,

Please don't top post!

On 01/01/2018 06:17 AM, Bear Giles wrote:
​You don't need to store 25:20 in the database though - your app can 
use a window that treats a day as "from 5 am today until 5 am 
tomorrow" and adds 24:00 to the times for tomorrow.​


Bear

On Sat, Dec 30, 2017 at 2:25 PM, Gavin Flower 
mailto:gavinflo...@archidevsys.co.nz>> 
wrote:


On 12/31/2017 03:07 AM, Dave Cramer wrote:

We are having a discussion on the jdbc project about dealing
with 24:00:00.

https://github.com/pgjdbc/pgjdbc/pull/992#issuecomment-354507612


Dave Cramer


In Dublin (I was there 2001 to 2004), Time tables show buses just
after midnight, such as 1:20am as running at the time 2520 - so
there are visible close to the end of the day.  If you are looking
for buses around midnight this is very user friendly - better than
looking at the other end of the time table for 0120.

I think logically that 24:00:00 is exactly one day later than
00:00:00 - but I see from following the URL, that there are other
complications...


Cheers,
Gavin



Sorry, I did not mean to imply that the data base should store values 
24:00:00 and greater!


For something like a time table, values like "24:00" and "25:20" should 
be generated by software where (and if) appropriate.



Cheers,
Gavin

Re: Contributing with code

[Robert Haas]

On Thu, Dec 28, 2017 at 5:42 PM, Craig Ringer  wrote:
> Well, it's arguably solveable, if the solution is to allow at least limited
> cross-database access to catalog relations.

I think this is a bad idea.  It's bound to add complexity and
fragility to the system and I don't think it's worth it.

Also, let's delete the TODO list.  People keep using it as a source of
project ideas, and that's bad.

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

Re: Re: PATCH: Configurable file mode mask

[Robert Haas]

On Thu, Dec 28, 2017 at 2:36 PM, David Steele  wrote:
> Attached is a new patch set that should address various concerns raised in
> this thread.
>
> 1) group-access-v3-01-mkdir.patch
>
> Abstracts all mkdir calls in the backend into a MakeDirectory() function
> implemented in fd.c.  This did not get committed in September as part of
> 0c5803b450e but I still think it has value.  However, I have kept it
> separate to reduce noise in the second patch.  The mkdir() calls could also
> be modified to use PG_DIR_MODE_DEFAULT with equivalent results.

+/*
+ * Default mode for created directories, unless something else is specified
+ * using the MakeDirectoryPerm() function variant.
+ */
+#define PG_DIR_MODE_DEFAULT(S_IRWXU)
+#define PG_MODE_MASK_DEFAULT   (S_IRWXG | S_IRWXO)

There's only one comment here, but there are two constants that do
different things.

Also, this hunk gets removed again by 02, which is probably OK, but 02
adds the replacement stuff in a different part of the file, which
seems not as good.

I think MakeDirectory() is a good wrapper, but isn't
MakeDirectoryPerm() sort of silly?

> 2) group-access-v3-02-group.patch
>
> This is a "GUC-less" implementation of group read access that depends on the
> mode of the $PGDATA directory to determine which mode to use for subsequent
> writes.  The initdb option is preserved to allow group access to be enabled
> when the cluster is initialized.
>
> Only two modes are allowed (700, 750) and the error message on startup is
> hard-coded to address translation concerns.

+   umask(~(stat_buf.st_mode & PG_DIR_MODE_DEFAULT));

Hmm, I wonder if this is going to be 100% portable.  Maybe some
obscure platform won't like an argument with all the high bits set.

Also, why should we break what's now one #if into two?  That seems
like it's mildly more complicated for no gain.

+(These files can confuse pg_ctl.)  If group read
+access is enabled on the data directory and an unprivileged user in the
+PostgreSQL group is performing the backup, then
+postmaster.pid will not be readable and must be
+excluded.

Is that actually the behavior we want?

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

Re: [HACKERS] Re: [HACKERS] generated columns

[Joe Conway]

On 12/31/2017 09:38 AM, Peter Eisentraut wrote:
> On 12/30/17 16:04, Joe Conway wrote:
>> +
>> + The generation expression can refer to other columns in the table, but
>> + not other generated columns.  Any functions and operators used must be
>> + immutable.  References to other tables are not allowed.
>> +
>> 
>> Question -- when the "stored" kind of generated column is implemented,
>> will the immutable restriction be relaxed? I would like, for example, be
>> able to have a stored generated column that executes now() whenever the
>> row is written/rewritten.



> Maybe some of this could be relaxed at some point, but we would have to
> think it through carefully.  For now, a trigger would still be the best
> implementation for your use case, I think.

Sure, but generated column behavior in general can be implemented via
trigger.

Anyway, I have seen requests for change data capture
(https://en.wikipedia.org/wiki/Change_data_capture) in Postgres which is
apparently available in our competition without requiring the use of
triggers. Perhaps that is yet a different feature, but I was hopeful
that this mechanism could be used to achieve it.

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



signature.asc
Description: OpenPGP digital signature

Re: What does Time.MAX_VALUE actually represent?

[Tom Lane]

Peter Eisentraut  writes:
> select timestamp '2017-12-30 24:00:00';
> returns
> 2017-12-31 00:00:00
> which makes some sense.

> I don't know why we accept that and not '24:00:01' and beyond, but it's
> probably historical.

We also accept

regression=# select timestamp '2017-12-30 23:59:60';
  timestamp  
-
 2017-12-31 00:00:00
(1 row)

which is maybe a little bit more defensible as a common behavior
to allow for leap seconds.  (It's far from a correct implementation of
leap seconds, of course, but I think most versions of mktime() and
friends do likewise.)

Digging into the git history, it looks like this choice dates to

commit a93bf4503ffc6d7cd6243a6324fb2ef206b10adf
Author: Bruce Momjian 
Date:   Fri Oct 14 11:47:57 2005 +

Allow times of 24:00:00 to match rounding behavior:

regression=# select '23:59:59.9'::time(0);
   time
--
 24:00:00
(1 row)

This is bad because:

regression=# select '24:00:00'::time(0);
ERROR:  date/time field value out of range: "24:00:00"

The last example now works.


There may at the time have been some argument about imprecision of
float timestamps involved, but it works the same way today once
you exceed the precision of our integer timestamps:

regression=# select time '23:59:59.99';
  time   
-
 23:59:59.99
(1 row)

regression=# select time '23:59:59.999';
   time   
--
 24:00:00
(1 row)

If we didn't allow '24:00:00' as a valid value then we'd need to
throw an error for '23:59:59.999', which doesn't seem nice.

regards, tom lane

Re: [HACKERS] Re: [HACKERS] generated columns

[Peter Eisentraut]

On 12/30/17 16:04, Joe Conway wrote:
> +
> + The generation expression can refer to other columns in the table, but
> + not other generated columns.  Any functions and operators used must be
> + immutable.  References to other tables are not allowed.
> +
> 
> Question -- when the "stored" kind of generated column is implemented,
> will the immutable restriction be relaxed? I would like, for example, be
> able to have a stored generated column that executes now() whenever the
> row is written/rewritten.

That restriction is from the SQL standard, and I think it will stay.
The virtual vs. stored choice is an optimization, but not meant to
affect semantics.  For example, you might want to automatically
substitute a precomputed generated column into an expression, but that
will become complicated and confusing if the expression is not
deterministic.

Another problem with your example is that a stored generated column
would only be updated if a column it depends on is updated.  So a column
whose generation expression is just now() would never get updated.

Maybe some of this could be relaxed at some point, but we would have to
think it through carefully.  For now, a trigger would still be the best
implementation for your use case, I think.

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

[Patch] Checksums for SLRU files

[Ivan Kartyshov]

Hello, I`d like to show my implementation of SLRU file protection with 
checksums.


It only has effect if checksums on database are enabled.
Detection of a checksum failure during a read normally causes PostgreSQL 
to report an error. Setting ignore_slru_checksum_failure to on causes 
the system to ignore the failure (but still report a warning), and  
continue processing. It is similary like ignore_checksum_failure but for 
some slru files. The default setting is true, and it can only be changed 
by a database admin. For changes, use ALTER SYSTEM and reload 
configuration:

ALTER SYSTEM SET ignore_slru_checksum_failure = off;
SELECT pg_reload_conf();

Impementation:
1) Checksum writes in last 2 bytes of every page
2) Checksum calculates and write down in page happens when page writes 
on disk (same as relation does it)
3) Checking checksum happens same as relation does it, on 
Read\PhysicalRead when GUC ignore_slru_checksum_failure = false

{default = true}.

I would like to hear your thoughts over my patch.

--
Ivan Kartyshov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Companydiff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index aeda826..8501dd2 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -8394,6 +8394,29 @@ LOG:  CleanUpLock: deleting: lock(0xb7acd844) id(24688,24696,0,0,0,1)
   
  
 
+
+  ignore_slru_checksum_failure (boolean)
+  
+   ignore_slru_checksum_failure configuration parameter
+  
+  
+  
+   
+Only has effect if  are enabled.
+   
+   
+Detection of a checksum failure during a read normally causes
+PostgreSQL to report an error. Setting ignore_slru_checksum_failure
+to on causes the system to ignore the failure (but still report a warning), and
+continue processing. Similary like ignore_checksum_failure but for
+not relation files. The default setting is true, and it can only be
+changed by a superuser. For changes, use ALTER SYSTEM and reload configuration:
+ALTER SYSTEM SET ignore_slru_checksum_failure = off; 
+SELECT pg_reload_conf(); 
+   
+  
+ 
+
 
   zero_damaged_pages (boolean)
   
diff --git a/src/backend/access/transam/clog.c b/src/backend/access/transam/clog.c
index a3e2b12..87ed09c 100644
--- a/src/backend/access/transam/clog.c
+++ b/src/backend/access/transam/clog.c
@@ -58,7 +58,7 @@
 /* We need two bits per xact, so four xacts fit in a byte */
 #define CLOG_BITS_PER_XACT	2
 #define CLOG_XACTS_PER_BYTE 4
-#define CLOG_XACTS_PER_PAGE (BLCKSZ * CLOG_XACTS_PER_BYTE)
+#define CLOG_XACTS_PER_PAGE ((BLCKSZ - CHKSUMSZ) * CLOG_XACTS_PER_BYTE)
 #define CLOG_XACT_BITMASK	((1 << CLOG_BITS_PER_XACT) - 1)
 
 #define TransactionIdToPage(xid)	((xid) / (TransactionId) CLOG_XACTS_PER_PAGE)
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 7142ece..35c317e 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -106,7 +106,7 @@
  */
 
 /* We need four bytes per offset */
-#define MULTIXACT_OFFSETS_PER_PAGE (BLCKSZ / sizeof(MultiXactOffset))
+#define MULTIXACT_OFFSETS_PER_PAGE ((BLCKSZ - CHKSUMSZ) / sizeof(MultiXactOffset))
 
 #define MultiXactIdToOffsetPage(xid) \
 	((xid) / (MultiXactOffset) MULTIXACT_OFFSETS_PER_PAGE)
@@ -2039,7 +2039,6 @@ TrimMultiXact(void)
 	{
 		int			slotno;
 		MultiXactOffset *offptr;
-
 		slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, nextMXact);
 		offptr = (MultiXactOffset *) MultiXactOffsetCtl->shared->page_buffer[slotno];
 		offptr += entryno;
diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
index 9dd7719..b7a154c 100644
--- a/src/backend/access/transam/slru.c
+++ b/src/backend/access/transam/slru.c
@@ -58,7 +58,18 @@
 #include "storage/fd.h"
 #include "storage/shmem.h"
 #include "miscadmin.h"
+#include "utils/guc.h"
 
+#include "storage/checksum.h"
+#include "utils/memutils.h"
+
+/*
+ * GUC variable
+ * Set from backend:
+ * alter system set ignore_slru_checksum_failure = on/off;
+ * select pg_reload_conf();
+ */
+bool		ignore_slru_checksum_failure = true;
 
 #define SlruFileName(ctl, path, seg) \
 	snprintf(path, MAXPGPATH, "%s/%04X", (ctl)->Dir, seg)
@@ -376,6 +387,8 @@ SimpleLruReadPage(SlruCtl ctl, int pageno, bool write_ok,
   TransactionId xid)
 {
 	SlruShared	shared = ctl->shared;
+	int			checksum;
+	static char *pageCopy = NULL;
 
 	/* Outer loop handles restart if we must wait for someone else's I/O */
 	for (;;)
@@ -426,6 +439,22 @@ SimpleLruReadPage(SlruCtl ctl, int pageno, bool write_ok,
 		/* Do the read */
 		ok = SlruPhysicalReadPage(ctl, pageno, slotno);
 
+		if (pageCopy == NULL)
+			pageCopy = MemoryContextAlloc(TopMemoryContext, BLCKSZ);
+
+		memcpy(pageCopy, shared->page_buffer[slotno], BLCKSZ);
+
+		checksum = pg_getchecksum_slru_page(pageCopy);
+
+		if ( DataChecksumsEnabled() && (checksum

Re: What does Time.MAX_VALUE actually represent?

[Peter Eisentraut]

On 12/30/17 09:07, Dave Cramer wrote:
> We are having a discussion on the jdbc project about dealing with 24:00:00.
> 
> https://github.com/pgjdbc/pgjdbc/pull/992#issuecomment-354507612

select timestamp '2017-12-30 24:00:00';

returns

2017-12-31 00:00:00

which makes some sense.

I don't know why we accept that and not '24:00:01' and beyond, but it's
probably historical.

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

Re: What does Time.MAX_VALUE actually represent?

[Bear Giles]

​You don't need to store 25:20 in the database though - your app can use a
window that treats a day as "from 5 am today until 5 am tomorrow" and adds
24:00 to the times for tomorrow.​

Bear

On Sat, Dec 30, 2017 at 2:25 PM, Gavin Flower  wrote:

> On 12/31/2017 03:07 AM, Dave Cramer wrote:
>
>> We are having a discussion on the jdbc project about dealing with
>> 24:00:00.
>>
>> https://github.com/pgjdbc/pgjdbc/pull/992#issuecomment-354507612
>>
>> Dave Cramer
>>
>
> In Dublin (I was there 2001 to 2004), Time tables show buses just after
> midnight, such as 1:20am as running at the time 2520 - so there are visible
> close to the end of the day.  If you are looking for buses around midnight
> this is very user friendly - better than looking at the other end of the
> time table for 0120.
>
> I think logically that 24:00:00 is exactly one day later than 00:00:00 -
> but I see from following the URL, that there are other complications...
>
>
> Cheers,
> Gavin
>
>
>

Re: [PATCH] GET DIAGNOSTICS FUNCTION_NAME

[Tom Lane]

Yugo Nagata  writes:
> Attached is a patch to implement a feature to get the current function
> name by GET DIAGNOSTICS in PL/pgSQL function.

While this is certainly not a very large patch, it's still code that
we'd have to maintain forever, so I think it's appropriate to ask
some harder questions before accepting it.

1. I'm having a hard time visualizing an actual concrete use case for
this --- exactly when would a function not know its own name?  Neither
your "our client wanted it" justification nor the cited stackoverflow
question provide anything close to an adequate rationale.  I can think of
concrete uses for an operation like "give me the name of my immediate
caller", but that's not what this is.

2. The specific semantics you've chosen --- in effect, regprocedureout
results --- seem to be more because that was already available than
anything else.  I can imagine wanting just the bare name, or the
schema-qualified name, or even the numeric OID (if we're in the
business of introspection, being able to look up the function's own
pg_proc entry might be useful).  I'm not proposing that we offer
all those variants, certainly, but without concrete use cases it's
pretty hard to be sure we picked the most useful behavior.

3. In connection with #2, I'm dubious that FUNCTION_NAME is le mot
juste, because that would seem to imply that it is just the name,
which it isn't.  If we stick with the regprocedureout semantics
I'd be inclined to propose FUNCTION_SIGNATURE.

regards, tom lane

Re: [HACKERS] Parallel tuplesort (for parallel B-Tree index creation)

[Peter Geoghegan]

On Tue, Dec 12, 2017 at 2:09 AM, Rushabh Lathia
 wrote:
>> I now believe that index_create() should reject catalog parallel
>> CREATE INDEX directly, just as it does for catalog CREATE INDEX
>> CONCURRENTLY. That logic should be generic to all AMs, since the
>> reasons for disallowing catalog parallel index builds are generic.
>>
>
> Sorry I didn't get this, reject means? you mean it should throw an error
> catalog parallel CREATE INDEX? or just suggesting to set the
> ParallelWorkers and may be LeaderAsWorker from index_create()
> or may be index_build()?

I mean that we should be careful to make sure that AM-generic parallel
CREATE INDEX logic does not end up in a specific AM (nbtree).

The patch *already* refuses to perform a parallel CREATE INDEX on a
system catalog, which is what I meant by reject (sorry for being
unclear). The point is that that's due to a restriction that has
nothing to do with nbtree in particular (just like the CIC restriction
on catalogs), so it should be performed within index_build(). Just
like the similar CONCURRENTLY-on-a-catalog restriction, though without
throwing an error, since of course the user doesn't explicitly ask for
a parallel CREATE INDEX at any point (unlike CONCURRENTLY).

Once we go this way, the cost model has to be called at that point,
too. We already have the AM-specific "OldIndex->rd_rel->relam ==
BTREE_AM_OID" tests within cluster.c, even though theoretically
another AM might be involved with CLUSTER in the future, which this
seems similar to.

So, I propose the following (this is a rough outline):

* Add new IndexInfo files after ii_Concurrent/ii_BrokenHotChain  --
ii_ParallelWorkers and ii_LeaderAsWorker.

* Call plan_create_index_workers() within index_create(), assigning to
ii_ParallelWorkers, and fill in ii_LeaderAsWorker from the
parallel_leader_participation GUC. Add comments along the lines of
"only nbtree supports parallel builds". Test the index with a
"heapRelation->rd_rel->relam == BTREE_AM_OID" to make this work.
Otherwise, assign zero to ii_ParallelWorkers (and leave
ii_LeaderAsWorker as false).

* For builds on catalogs, or builds using other AMs, don't let
parallelism go ahead by immediately assigning zero to
ii_ParallelWorkers within index_create(), near where the similar CIC
test occurs already.

What do you think of that?

>> Do you think that the main part of the cost model needs to care about
>> parallel_leader_participation, too?
>>
>> compute_parallel_worker() assumes that the caller is planning a
>> parallel-sequential-scan-alike thing, in the sense that the leader
>> only acts like a worker in cases that probably don't have many
>> workers, where the leader cannot keep itself busy as a leader. That's
>> actually quite different to parallel CREATE INDEX, because the
>> leader-as-worker state will behave in exactly the same way as a worker
>> would, no matter how many workers there are. The leader process is
>> guaranteed to give its full attention to being a worker, because it
>> has precisely nothing else to do until workers finish. This makes me
>> think that we may need to immediately do something with the result of
>> compute_parallel_worker(), to consider whether or not a
>> leader-as-worker state should be used, despite the fact that no
>> existing compute_parallel_worker() caller does anything like this.
>>
>
> I agree with you. compute_parallel_worker() mainly design for the
> scan-alike things. Where as parallel create index is different in a
> sense where leader has as much power as worker.  But at the same
> time I don't see any side effect or negative of that with PARALLEL
> CREATE INDEX.  So I am more towards not changing that aleast
> for now - as part of this patch.

I've also noticed is that there is little to no negative effect on
CREATE INDEX duration from adding new workers past the point where
adding more workers stops making the build faster. It's quite clear.
And, in general, there isn't all that much theoretical justification
for the cost model (it's essentially the same as any other parallel
scan), which doesn't seem to matter much. So, I agree that it doesn't
really matter in practice, but disagree that it should not still be
changed -- the justification may be a little thin, but I think that we
need to stick to it. There should be a theoretical justification for
the cost model that is coherent in the wider context of costs models
for parallelism in general. It should not be arbitrarily inconsistent
just because it apparently doesn't matter that much. It's easy to fix
-- let's just fix it.

-- 
Peter Geoghegan

Re: Fix a Oracle-compatible instr function in the documentation

[Tatsuo Ishii]

> The documentation of PL/pgSQL provides sample codes of Oracle-compatible
> instr functions. However, the behaviour is a little differet.
> Oracle's instr raises an error when the forth argument value is less than
> zero, but the sample code returns zero. This patch fixes this.

Your patch fixes only instr(string varchar, string_to_search varchar, beg_index 
integer).
However in the doc there is another instr(string varchar, string_to_search 
varchar, beg_index integer, occur_index integer).

Shouldn't this be fixed as well?

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

Re: [PROPOSAL] Shared Ispell dictionaries

[Arthur Zakirov]

Hello, hackers,

On Tue, Dec 26, 2017 at 07:48:27PM +0300, Arthur Zakirov wrote:
> The patch will be ready and added into the 2018-03 commitfest.
> 

I attached the patch itself.

0001-Fix-ispell-memory-handling.patch:

Some strings are allocated via compact_palloc0(). But they are not
persistent, so they should be allocated using temporary memory context.
Also a couple strings are not released if .aff file had new format.

0002-Retreive-shmem-location-for-ispell.patch:

Adds ispell_shmem_location() function which look for location for a
dictionary using .dict and .aff file names. If the location haven't been
allocated in DSM earlier, allocate it. Shared hash table is used here to
search the location.

Maximum number of elements of hash table is NUM_DICTIONARIES=20 now. It
will be better to use a GUC-variable. Also if the number of elements
reached the limit then it will be good to use backend's local memory
instead of shared.

0003-Store-ispell-structures-in-shmem.patch:

Introduces IspellDictBuild and IspellDictData structures, removes
IspellDict structure. IspellDictBuild is used during building the
dictionary, if it haven't been allocated in DSM earlier, within
dispell_build() function. IspellDictBuild has a pointer to
IspellDictData structure, which will be filled with persistent data.

After building the dictionary IspellDictData is copied into
DSM location and temporary data of IspellDictBuild is released.

All prefix trees are stored as a flat array now. Those arrays are
allocated and stored using NodeArray struct now. Required node can be
retreied by node offset. AffixData and Affix arrays have additional
offset array to retreive an element by index.

Affix field (array of AFFIX) of IspellDictBuild is persistent data also. But it 
is
constructed as a temporary array first, Affix array need to be sorted
via qsort() within NISortAffixes().

So IspellDictData stores:
- AffixData - array of strings, access via AffixDataOffset
- Affix - array of AFFIX, access via AffixOffset
- DictNodes, PrefixNodes, SuffixNodes - prefix trees as a plain array
- CompoundAffix - array of CMPDAffix sequential access

I had to remove compact_palloc0() added by Pavel in
3e5f9412d0a818be77c974e5af710928097b91f3. Ispell dictionary doesn't need
such allocation anymore. It was used to allocate a little locations. I
will definity check performance of Czech dictionary.


There are issues to do:
- add the GUC-variable for hash table limit
- fix bugs
- improve comments
- performance testing

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
diff --git a/src/backend/tsearch/spell.c b/src/backend/tsearch/spell.c
index 9a09ffb20a..6617c2cf05 100644
--- a/src/backend/tsearch/spell.c
+++ b/src/backend/tsearch/spell.c
@@ -498,7 +498,7 @@ NIAddSpell(IspellDict *Conf, const char *word, const char 
*flag)
Conf->Spell[Conf->nspell] = (SPELL *) tmpalloc(SPELLHDRSZ + 
strlen(word) + 1);
strcpy(Conf->Spell[Conf->nspell]->word, word);
Conf->Spell[Conf->nspell]->p.flag = (*flag != '\0')
-   ? cpstrdup(Conf, flag) : VoidString;
+   ? MemoryContextStrdup(Conf->buildCxt, flag) : VoidString;
Conf->nspell++;
 }
 
@@ -1040,7 +1040,7 @@ setCompoundAffixFlagValue(IspellDict *Conf, 
CompoundAffixFlag *entry,
entry->flag.i = i;
}
else
-   entry->flag.s = cpstrdup(Conf, s);
+   entry->flag.s = MemoryContextStrdup(Conf->buildCxt, s);
 
entry->flagMode = Conf->flagMode;
entry->value = val;
@@ -1536,6 +1536,9 @@ nextline:
return;
 
 isnewformat:
+   pfree(recoded);
+   pfree(pstr);
+
if (oldformat)
ereport(ERROR,
(errcode(ERRCODE_CONFIG_FILE_ERROR),
diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c
index 2d1ed143e0..86a6df131b 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -44,6 +44,7 @@
 #include "storage/procsignal.h"
 #include "storage/sinvaladt.h"
 #include "storage/spin.h"
+#include "tsearch/ts_shared.h"
 #include "utils/backend_random.h"
 #include "utils/snapmgr.h"
 
@@ -150,6 +151,7 @@ CreateSharedMemoryAndSemaphores(bool makePrivate, int port)
size = add_size(size, SyncScanShmemSize());
size = add_size(size, AsyncShmemSize());
size = add_size(size, BackendRandomShmemSize());
+   size = add_size(size, TsearchShmemSize());
 #ifdef EXEC_BACKEND
size = add_size(size, ShmemBackendArraySize());
 #endif
@@ -271,6 +273,11 @@ CreateSharedMemoryAndSemaphores(bool makePrivate, int port)
AsyncShmemInit();
BackendRandomShmemInit();
 
+   /*
+* Set up shared memory to tsearch
+*/
+   TsearchShmemInit();
+
 #ifdef EXEC_BACKEND
 
/*
diff --git a/src/backend/storage/lmgr/lwlock.c 
b/src/backend/storage/lmgr/lwlock.c
index eab98b0760..d8c8cc8cc3

Re: Faster inserts with mostly-monotonically increasing values

[Andrey Borodin]

Hi!
> 31 дек. 2017 г., в 11:44, Pavan Deolasee  
> написал(а):
> On a per-session basis, we cache the last heap block used for inserts and try 
> to use the same block for subsequent inserts.
+1, from my POV this is good idea and it's cool that it already has 
implementation.
I'd suggest to do not 1\1 split for these pages, to keep tree lower. Like 
fillfactor/(1-fillfactor) split. Also, this will make splits less frequent.

Best regards, Andrey Borodin.

Re: MCV lists for highly skewed distributions

[John Naylor]

I wrote:

> On 12/28/17, Jeff Janes  wrote:
>> I think that perhaps maxmincount should also use the dynamic
>> values_cnt_remaining rather than the static one.  After all, things
>> included in the MCV don' get represented in the histogram.  When I've
>> seen
>> planning problems due to skewed distributions I also usually see
>> redundant
>> values in the histogram boundary list which I think should be in the MCV
>> list instead. But I have not changed that here, pending discussion.
>
> I think this is also a good idea, but I haven't thought it through. If
> you don't go this route, I would move this section back out of the
> loop as well.

I did some quick and dirty testing of this, and I just want to note
that in this case, setting mincount to its hard-coded minimum must
come after setting it to maxmincount, since now maxmincount can go
arbitrarily low.

I'll be travelling for a few days, but I'll do some testing on some
data sets soon. While looking through the archives for more info, I
saw this thread

https://www.postgresql.org/message-id/32261.1496611829%40sss.pgh.pa.us

which showcases the opposite problem: For more uniform distributions,
there are too many MCVs. Not relevant to your problem, but if I have
time I'll try my hand at testing an approach suggested in that thread
at the same time I test your patch and see how it interacts.

-John Naylor

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

[Dmitry Dolgov]

> On 31 December 2017 at 06:55, Marina Polyakova 
wrote:
>
> Secondly, here there's a sixth version of the patch for the
precalculation of
> stable or immutable functions, stable or immutable operators and other
> nonvolatile expressions.

Thanks for your patch, looks quite interesting!

> To not send big patch I have split it (that's why version starts with the
> first again) and here I send infrastructure patch which includes:

Yeah, but it's still 18k lines :) After the first quick glance I have a few
small questions.

If I call a stable function from a query and subquery, looks like it's
cached:

```
=# select stable_with_int(1) from (select stable_with_int(1) from x) q;
NOTICE:  0: stable with int
LOCATION:  exec_stmt_raise, pl_exec.c:3353
 stable_with_int
-
   1
   1
   1
   1
(4 rows)
```

But the same from CTE works different, is it supposed to be like that?

```
=# with data as (select stable_with_int(1) from x) select
stable_with_int(1) from data;
NOTICE:  0: stable with int
LOCATION:  exec_stmt_raise, pl_exec.c:3353
NOTICE:  0: stable with int
LOCATION:  exec_stmt_raise, pl_exec.c:3353
 stable_with_int
-
   1
   1
   1
   1
(4 rows)
```

Also I see this pattern quite some time, maybe it makes sense to move it to
a function?

```
+ /* create and return CachedExpr */
+ CachedExpr *new_node = makeNode(CachedExpr);
+ new_node->subexpr = (CacheableExpr *) current_node;
+
+ context->root->hasCachedExpr = true;
+
+ return (Node *) new_node;
```

Sample values for pg_stat_statements

[Vik Fearing]

Often when looking through pg_stat_statements, it would be nice to have
some sample values for the constants and parameters.  This patch
implements that by taking the values from the first execution of the
normalized query.

To keep things reasonable, there is a limit on how big the parameters
can be.

This patch is based off of 5303ffe71b.
-- 
Vik Fearing  +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index 39b368b70e..fae5c29cef 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -4,9 +4,13 @@ MODULE_big = pg_stat_statements
 OBJS = pg_stat_statements.o $(WIN32RES)
 
 EXTENSION = pg_stat_statements
-DATA = pg_stat_statements--1.4.sql pg_stat_statements--1.4--1.5.sql \
-	pg_stat_statements--1.3--1.4.sql pg_stat_statements--1.2--1.3.sql \
-	pg_stat_statements--1.1--1.2.sql pg_stat_statements--1.0--1.1.sql \
+DATA = pg_stat_statements--1.6.sql \
+	pg_stat_statements--1.5--1.6.sql \
+	pg_stat_statements--1.4--1.5.sql \
+	pg_stat_statements--1.3--1.4.sql \
+	pg_stat_statements--1.2--1.3.sql \
+	pg_stat_statements--1.1--1.2.sql \
+	pg_stat_statements--1.0--1.1.sql \
 	pg_stat_statements--unpackaged--1.0.sql
 PGFILEDESC = "pg_stat_statements - execution statistics of SQL statements"
 
diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index 5318c3550c..f032db98c2 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -95,25 +95,25 @@ EXECUTE pgss_test(1);
 (1 row)
 
 DEALLOCATE pgss_test;
-SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
-   query   | calls | rows 
+---+--
- PREPARE pgss_test (int) AS SELECT $1, $2 LIMIT $3 | 1 |1
- SELECT $1+| 4 |4
-  +|   | 
-   AS "text"   |   | 
- SELECT $1 + $2| 2 |2
- SELECT $1 + $2 + $3 AS "add"  | 3 |3
- SELECT $1 AS "float"  | 1 |1
- SELECT $1 AS "int"| 2 |2
- SELECT $1 AS i UNION SELECT $2 ORDER BY i | 1 |2
- SELECT $1 || $2   | 1 |1
- SELECT pg_stat_statements_reset() | 1 |1
- WITH t(f) AS (   +| 1 |2
-   VALUES ($1), ($2)  +|   | 
- )+|   | 
-   SELECT f FROM t ORDER BY f  |   | 
- select $1::jsonb ? $2 | 1 |1
+SELECT query, calls, rows, consts, params, param_types FROM pg_stat_statements ORDER BY query COLLATE "C";
+   query   | calls | rows |consts| params | param_types 
+---+---+--+--++-
+ PREPARE pgss_test (int) AS SELECT $1, $2 LIMIT $3 | 1 |1 | [2:3]={'test',1} | {1}| {integer}
+ SELECT $1+| 4 |4 | {'hello'}|| 
+  +|   |  |  || 
+   AS "text"   |   |  |  || 
+ SELECT $1 + $2| 2 |2 | {3,3}|| 
+ SELECT $1 + $2 + $3 AS "add"  | 3 |3 | {1,1,1}  || 
+ SELECT $1 AS "float"  | 1 |1 | {2.0}|| 
+ SELECT $1 AS "int"| 2 |2 | {1}  || 
+ SELECT $1 AS i UNION SELECT $2 ORDER BY i | 1 |2 | {1,2}|| 
+ SELECT $1 || $2   | 1 |1 | {"' '","' !'"}   || 
+ SELECT pg_stat_statements_reset() | 1 |1 |  || 
+ WITH t(f) AS (   +| 1 |2 | {1.0,2.0}|| 
+   VALUES ($1), ($2)  +|   |  |  || 
+ )+|   |  |  || 
+   SELECT f FROM t ORDER BY f   

Re: [PATCH] GET DIAGNOSTICS FUNCTION_NAME

[Fabien COELHO]

Attached is a patch to implement a feature to get the current function 
name by GET DIAGNOSTICS in PL/pgSQL function.


Shouldn't it be tested somewhere?

--
Fabien.

Re: Faster inserts with mostly-monotonically increasing values

[Peter Geoghegan]

On Sun, Dec 31, 2017 at 6:44 AM, Pavan Deolasee
 wrote:
> Here is a patch that implements the idea. If the last insert happens to be
> in the rightmost block of an index, then we cache the block and check that
> first for the next insert. For the cached block to be usable for the insert,
> it should still be the rightmost, the to-be-inserted key should go into the
> cached block and there is enough free space in the block. If any of these
> conditions do not meet, we fall back to the regular code path, i.e. descent
> down the index from the top.

I'd be particularly concerned about page deletion/page recycling still
being correct with the patch, especially because it's hard to test
anything there. The P_ISLEAF() part of your fastpath's test hints at
this -- why should it not *always* be a leaf page (surely you should
be sure that the page cannot be recycled anyway)? I also have my
doubts about unique index enforcement remaining correct with the patch
when there are many physical duplicates, to the extent that more than
a single leaf page is needed for a single value.

Maybe you should do something with page LSN here -- cache LSN
alongside block number, and have a non-matching LSN invalidate the
test.

How many clients are involved with your benchmark?

> So as the size of the index increases, the benefits of the patch also tend
> to increase. This is most likely because as the index becomes taller and
> taller, the costs associated with index descent becomes higher.

FWIW, I think that int4 indexes have to be extremely large before they
exceed 4 levels (where the leaf level counts as a level). My
conservative back-of-an-envelope estimate is 9 billion index tuples.

-- 
Peter Geoghegan

Re: Logical decoding fast-forward and slot advance

[Magnus Hagander]

Hi!

Excellent!

Just a quick note on my stab at it - it was physical only, which is more
limited than this of course. My plan was to clean it up based on feedback
in the next couple of days for the Jan cf, but with this submission in i
think the effort is better directed there. I'll keep mine around as a
backup in case something shows up with yours so it won't make it on time
since mine is simpler.

/Magnus



On Dec 31, 2017 11:44, "Petr Jelinek"  wrote:

Hi,

Attached is patch which adds ability to do fast-forwarding while
decoding. That means wal is consumed as fast as possible and changes are
not given to output plugin for sending. The implementation is less
invasive than I originally though it would be. Most of it is just
additional filter condition in places where we would normally filter out
changes because we don't yet have full snapshot.

This is useful for multiple things. It enables us to do the replication
slot advance for both physical and logical slots, something that Magnus
took stab at some time ago, but does not seem like it went anywhere
(this is useful for replication tooling). This patch adds SQL visible
pg_replication_slot_advance() function for that use case.

It also makes second phase (after we reached SNAPBUILD_FULL_SNAPSHOT) of
replication slot creation faster, especially when there are big
transactions as the reorder buffer does not have to deal with data
changes and does not have to spill to disk.

And finally it will be useful for developing failover support of slots.

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

Re: [HACKERS] [PATCH] Lockable views

[Yugo Nagata]

Hi,

The updated patch is attached.

On Fri, 29 Dec 2017 23:39:39 +0900 (JST)
Tatsuo Ishii  wrote:

 
> The patch produces a warning.
> 
> /home/t-ishii/lock_view-v3.patch:542: trailing whitespace.
> -- Verify that we  can lock a auto-updatable views 
> warning: 1 line adds whitespace errors.

Fixed.

> 
> Your addition to the doc:
> +   Automatically updatable views (see )
> +   that do not have INSTEAD OF triggers or INSTEAD
> +   rules are also lockable. When a view is locked, its base relations are
> +   also locked recursively with the same lock mode.
> 
> does not mention about the point:
> 
> >> >> > 1) Leave as it is (ignore tables appearing in a subquery)

I added this point to the documentation.


Regards,

> 
> Best regards,
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese:http://www.sraoss.co.jp


-- 
Yugo Nagata 
diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml
index b2c7203..c2ed481 100644
--- a/doc/src/sgml/ref/lock.sgml
+++ b/doc/src/sgml/ref/lock.sgml
@@ -46,6 +46,14 @@ LOCK [ TABLE ] [ ONLY ] name [ * ]
   
 
   
+   Automatically updatable views (see )
+   that do not have INSTEAD OF triggers or INSTEAD
+   rules are also lockable. When a view is locked, its base relations are
+   also locked recursively with the same lock mode. Tables appearing in a
+   subquery are ignored and not locked.
+  
+
+  
When acquiring locks automatically for commands that reference
tables, PostgreSQL always uses the least
restrictive lock mode possible. LOCK TABLE
diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index 9fe9e02..80e00f7 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -19,15 +19,20 @@
 #include "commands/lockcmds.h"
 #include "miscadmin.h"
 #include "parser/parse_clause.h"
+#include "parser/parsetree.h"
 #include "storage/lmgr.h"
 #include "utils/acl.h"
 #include "utils/lsyscache.h"
 #include "utils/syscache.h"
+#include "rewrite/rewriteHandler.h"
+#include "access/heapam.h"
 
-static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait);
-static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode);
+static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid);
+static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode, Oid userid);
 static void RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid,
 			 Oid oldrelid, void *arg);
+static void LockViewRecurse(Oid reloid, Oid root_reloid, LOCKMODE lockmode, bool nowait);
+static void LockViewCheck(Relation view, Query *viewquery);
 
 /*
  * LOCK TABLE
@@ -62,8 +67,10 @@ LockTableCommand(LockStmt *lockstmt)
 		  RangeVarCallbackForLockTable,
 		  (void *) &lockstmt->mode);
 
-		if (recurse)
-			LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait);
+		if (get_rel_relkind(reloid) == RELKIND_VIEW)
+			LockViewRecurse(reloid, reloid, lockstmt->mode, lockstmt->nowait);
+		else if (recurse)
+			LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait, GetUserId());
 	}
 }
 
@@ -86,15 +93,17 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid,
 		return;	/* woops, concurrently dropped; no permissions
  * check */
 
-	/* Currently, we only allow plain tables to be locked */
-	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
+
+	/* Currently, we only allow plain tables or auto-updatable views to be locked */
+	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE &&
+		relkind != RELKIND_VIEW)
 		ereport(ERROR,
 (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table",
+ errmsg("\"%s\" is not a table or view",
 		rv->relname)));
 
 	/* Check permissions. */
-	aclresult = LockTableAclCheck(relid, lockmode);
+	aclresult = LockTableAclCheck(relid, lockmode, GetUserId());
 	if (aclresult != ACLCHECK_OK)
 		aclcheck_error(aclresult, ACL_KIND_CLASS, rv->relname);
 }
@@ -107,7 +116,7 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid,
  * multiply-inheriting children more than once, but that's no problem.
  */
 static void
-LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait)
+LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid)
 {
 	List	   *children;
 	ListCell   *lc;
@@ -120,7 +129,7 @@ LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait)
 		AclResult	aclresult;
 
 		/* Check permissions before acquiring the lock. */
-		aclresult = LockTableAclCheck(childreloid, lockmode);
+		aclresult = LockTableAclCheck(childreloid, lockmode, userid);
 		if (aclresult != ACLCHECK_OK)
 		{
 			char	   *relname = get_rel_name(childreloid);
@@ -157,15 +166,82 @@ LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait)
 			continue;
 		}
 
-		LockTableRecurse(childreloid, lockmode, nowait);
+		LockTableRecurse(childreloid, lockmode, nowait, userid);
 	}
 }
 
 /*
+ * Apply LOCK TABLE

Re: Why standby restores some WALs many times from archive?

[Michael Paquier]

On Sun, Dec 31, 2017 at 10:22:37AM +0300, Sergey Burladyan wrote:
> Michael Paquier  writes:
>> Don't be surprised if you get corrupted instances or that you
>> are not able to recover up to a consistent point if you need to roll in
>> a backup.
> 
> But only if archive was reboot unexpectedly, am I right?

Or unplugged, which happens quite often because of human mistakes.
Maintaining scripts that do backup and restore require quite a lot of
specialized knowledge. Solutions like barman, WAL-E or pgBackRest, or
even what in-core has with say pg_receivewal are battle-proven and have
hundreds of man-hours behind from many people. I suspect that there are
other gems hidden in your scripts. So be careful.
--
Michael


signature.asc
Description: PGP signature

Re: Fix a Oracle-compatible instr function in the documentation

[Yugo Nagata]

On Sun, 31 Dec 2017 17:52:49 +0900 (JST)
Tatsuo Ishii  wrote:

> >> Hi,
> >> 
> >> Attached is a patch to fix a very trivial issue of the documentation.
> >> 
> >> The documentation of PL/pgSQL provides sample codes of Oracle-compatible
> >> instr functions. However, the behaviour is a little differet.
> >> Oracle's instr raises an error when the forth argument value is less than
> >> zero, but the sample code returns zero. This patch fixes this.
> > 
> > Do we need treat this as a bug fix? If so, do we need to back patch as
> > well?

This is a little improvement of the documentation to reduce confusion of users
who work on migration from Oracle. I don't know whether this need tobe back
patched, so I'll leave a decision up to commiters.

> 
> I have added this to CF 2018-01.

Thank you.

> 
> Best regards,
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese:http://www.sraoss.co.jp


-- 
Yugo Nagata 

Logical decoding fast-forward and slot advance

[Petr Jelinek]

Hi,

Attached is patch which adds ability to do fast-forwarding while
decoding. That means wal is consumed as fast as possible and changes are
not given to output plugin for sending. The implementation is less
invasive than I originally though it would be. Most of it is just
additional filter condition in places where we would normally filter out
changes because we don't yet have full snapshot.

This is useful for multiple things. It enables us to do the replication
slot advance for both physical and logical slots, something that Magnus
took stab at some time ago, but does not seem like it went anywhere
(this is useful for replication tooling). This patch adds SQL visible
pg_replication_slot_advance() function for that use case.

It also makes second phase (after we reached SNAPBUILD_FULL_SNAPSHOT) of
replication slot creation faster, especially when there are big
transactions as the reorder buffer does not have to deal with data
changes and does not have to spill to disk.

And finally it will be useful for developing failover support of slots.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services
From f02cca94098cb7a29a94d409659a74349a8c9c2f Mon Sep 17 00:00:00 2001
From: Petr Jelinek 
Date: Fri, 29 Dec 2017 09:46:35 +0100
Subject: [PATCH] Add pg_replication_slot_advance() function

Advances replication slot to specified position. Works both with logical
and physical slots.

This also adds fast forward mode for logical decoding which is used by
the pg_replication_slot_advance() function as well as slot creation.
---
 doc/src/sgml/func.sgml |  19 +++
 src/backend/replication/logical/decode.c   |  40 +++--
 src/backend/replication/logical/logical.c  |  16 +-
 src/backend/replication/logical/logicalfuncs.c |   1 +
 src/backend/replication/slotfuncs.c| 199 +
 src/backend/replication/walsender.c|   1 +
 src/include/catalog/pg_proc.h  |   2 +
 src/include/replication/logical.h  |   8 +
 8 files changed, 269 insertions(+), 17 deletions(-)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 4dd9d029e6..67bad68bfc 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -19147,6 +19147,25 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());

   
 
+  
+   
+
+ pg_replication_slot_advance
+
+pg_replication_slot_advance(slot_name name, upto_lsn pg_lsn)
+   
+   
+(slot_name name, end_lsn pg_lsn)
+bool
+   
+   
+Advances the current confirmed position of a replication slot named
+slot_name. The slot will not be moved backwards,
+and it will not be moved beyond the current insert location.  Returns
+name of the slot and real position to which it was advanced to.
+   
+  
+
   

 
diff --git a/src/backend/replication/logical/decode.c b/src/backend/replication/logical/decode.c
index 486fd0c988..f502e34175 100644
--- a/src/backend/replication/logical/decode.c
+++ b/src/backend/replication/logical/decode.c
@@ -332,8 +332,10 @@ DecodeStandbyOp(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
 xl_invalidations *invalidations =
 (xl_invalidations *) XLogRecGetData(r);
 
-ReorderBufferImmediateInvalidation(
-   ctx->reorder, invalidations->nmsgs, invalidations->msgs);
+if (!ctx->fast_forward)
+	ReorderBufferImmediateInvalidation(ctx->reorder,
+	   invalidations->nmsgs,
+	   invalidations->msgs);
 			}
 			break;
 		default:
@@ -353,14 +355,19 @@ DecodeHeap2Op(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
 
 	ReorderBufferProcessXid(ctx->reorder, xid, buf->origptr);
 
-	/* no point in doing anything yet */
-	if (SnapBuildCurrentState(builder) < SNAPBUILD_FULL_SNAPSHOT)
+	/*
+	 * If we don't have snapshot or we are just fast-forwarding, there is no
+	 * point in decoding changes.
+	 */
+	if (SnapBuildCurrentState(builder) < SNAPBUILD_FULL_SNAPSHOT ||
+		ctx->fast_forward)
 		return;
 
 	switch (info)
 	{
 		case XLOG_HEAP2_MULTI_INSERT:
-			if (SnapBuildProcessChange(builder, xid, buf->origptr))
+			if (!ctx->fast_forward &&
+SnapBuildProcessChange(builder, xid, buf->origptr))
 DecodeMultiInsert(ctx, buf);
 			break;
 		case XLOG_HEAP2_NEW_CID:
@@ -408,8 +415,12 @@ DecodeHeapOp(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
 
 	ReorderBufferProcessXid(ctx->reorder, xid, buf->origptr);
 
-	/* no point in doing anything yet */
-	if (SnapBuildCurrentState(builder) < SNAPBUILD_FULL_SNAPSHOT)
+	/*
+	 * If we don't have snapshot or we are just fast-forwarding, there is no
+	 * point in decoding data changes.
+	 */
+	if (SnapBuildCurrentState(builder) < SNAPBUILD_FULL_SNAPSHOT ||
+		ctx->fast_forward)
 		return;
 
 	switch (info)
@@ -501,8 +512,12 @@ DecodeLogicalMsgOp(LogicalDecodingContext *ctx,

Re: [PATCH] GET DIAGNOSTICS FUNCTION_NAME

[Yugo Nagata]

On Sun, 31 Dec 2017 17:54:06 +0900 (JST)
Tatsuo Ishii  wrote:

> >> Attached is a patch to implement a feature to get the current function
> >> name by GET DIAGNOSTICS in PL/pgSQL function.
> > 
> > Could you add it to the nexf CF, I have not seen it there? Maybe the
> > deadline is tonight...
> 
> I have added this to the next CF.

Thank you.

> 
> Best regards,
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese:http://www.sraoss.co.jp


-- 
Yugo Nagata 

Re: [PATCH] GET DIAGNOSTICS FUNCTION_NAME

[Tatsuo Ishii]

>> Attached is a patch to implement a feature to get the current function
>> name by GET DIAGNOSTICS in PL/pgSQL function.
> 
> Could you add it to the nexf CF, I have not seen it there? Maybe the
> deadline is tonight...

I have added this to the next CF.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

Re: Fix a Oracle-compatible instr function in the documentation

[Tatsuo Ishii]

>> Hi,
>> 
>> Attached is a patch to fix a very trivial issue of the documentation.
>> 
>> The documentation of PL/pgSQL provides sample codes of Oracle-compatible
>> instr functions. However, the behaviour is a little differet.
>> Oracle's instr raises an error when the forth argument value is less than
>> zero, but the sample code returns zero. This patch fixes this.
> 
> Do we need treat this as a bug fix? If so, do we need to back patch as
> well?

I have added this to CF 2018-01.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

Re: [PATCH] GET DIAGNOSTICS FUNCTION_NAME

[Fabien COELHO]

Attached is a patch to implement a feature to get the current function
name by GET DIAGNOSTICS in PL/pgSQL function.


Could you add it to the nexf CF, I have not seen it there? Maybe the 
deadline is tonight...


--
Fabien.

Re: Better testing coverage and unified coding for plpgsql loops

[Pavel Stehule]

2017-12-30 22:46 GMT+01:00 Tom Lane :

> While I've been fooling around with plpgsql, I've been paying close
> attention to code coverage reports to make sure that the regression tests
> exercise all that new code.  It started to bug me that there were some
> serious gaps in the test coverage for existing code in pl_exec.c.
> One thing I noticed in particular was that coverage for the
> PLPGSQL_RC_EXIT/PLPGSQL_RC_RETURN/PLPGSQL_RC_CONTINUE management code
> in the various looping statements was nearly nonexistent, and coverage
> for integer FOR loops was pretty awful too (eg, not a single BY clause
> in the whole test corpus :-().  So I said to myself "let's fix that",
> and wrote up a new regression test file plpgsql_control.sql with a
> charter to test plpgsql's control structures.  I moved a few existing
> tests that seemed to fall into that charter into the new file, and
> added tests until I was happier with the state of the coverage report.
> The result is attached as plpgsql-better-code-coverage.patch.
>
> However, while I was doing that, it seemed like the tests I was adding
> were mighty repetitive, as many of them were just exactly the same thing
> adjusted for a different kind of loop statement.  And so I began to wonder
> why it was that we had five copies of the RC_FOO management logic, no two
> quite alike.  If we only had *one* copy then it would not seem necessary
> to have such duplicative test cases for it.  A bit of hacking later, and
> I had the management logic expressed as a macro, with only one copy for
> all five kinds of loop.  I verified it still passes the previous set of
> tests and then removed the ones that seemed redundant, yielding
> plpgsql-unify-loop-rc-code.patch below.  So what I propose actually
> committing is the combination of these two patches.
>
> Anyone feel like reviewing this, or should I just push it?  It seems
> pretty noncontroversial to me, but maybe I'm wrong about that.
>

I don't think any issue there. This macro is little bit massive, but
similar is used elsewhere.

+1

Pavel



> regards, tom lane
>
>