Re: PostgreSQL12 and older versions of OpenSSL

[Michael Paquier]

On Tue, Sep 24, 2019 at 12:43:17PM -0400, Tom Lane wrote:
> One thing I'm wondering is if it's safe to assume that TLS_MAX_VERSION
> will be defined whenever these other symbols are.  Looking in an
> 0.9.8x install tree, that doesn't seem to define any of them; while
> in 1.0.1e I see

Yeah, I could personally live with the argument of simplicity and just
say that trying to compile v12 with any version older than 0.9.8zc or
any version that does not have those symbols just does not work, and
that one needs to use the top of the released versions.

> ./tls1.h:#define TLS1_1_VERSION 0x0302
> ./tls1.h:#define TLS1_2_VERSION 0x0303
> ./tls1.h:#define TLS_MAX_VERSIONTLS1_2_VERSION
>
> So the patch seems okay for these two versions, but I have no data about
> intermediate OpenSSL versions.

More precisely, all those fields have been added by this upstream
commit, so the fields are present since 0.9.8zc:
commit: c6a876473cbff0fd323c8abcaace98ee2d21863d
author: Bodo Moeller 
date: Wed, 15 Oct 2014 04:18:29 +0200
Support TLS_FALLBACK_SCSV.

> BTW, the spacing in this patch seems rather random.

Indeed.

Now that I think about it, another method would be to rely on the fact
that a given version of OpenSSL does not support TLS 1.1 and 1.2.  So
we could also just add checks based on OPENSSL_VERSION_NUMBER and be
done with it.  And actually, looking at their tree TLS 1.1 and 2.2 are
not supported in 1.0.0 either.  1.0.1, 1.0.2, 1.1.0 and HEAD do
support them, but not TLS 1.3.

I would still prefer relying on TLS_MAX_VERSION though, as that's more
portable for future decisions, like the introduction of TLS1_3_VERSION
for which we have already some logic in be-secure-openssl.c.  And
updating this stuff would very likely get forgotten once OpenSSL adds
support for TLS 1.3...

There is another issue in the patch:
-#ifdef TLS1_3_VERSION
+#if defined(TLS1_3_VERSION)  &&  TLS1_2_VERSION <= TLS_MAX_VERSION
The second part of the if needs to use TLS1_3_VERSION.

I would also add more brackets around the extra conditions for
readability.
--
Michael


signature.asc
Description: PGP signature

Re: heapam_index_build_range_scan's anyvisible

[Ashwin Agrawal]

On Wed, Sep 25, 2019 at 1:52 PM Alvaro Herrera 
wrote:

> Sounds OK ... except that Travis points out that Ashwin forgot to patch
> contrib:
>
> make[4]: Entering directory
> '/home/travis/build/postgresql-cfbot/postgresql/contrib/amcheck'
> gcc -Wall -Wmissing-prototypes -Wpointer-arith
> -Wdeclaration-after-statement -Werror=vla -Wendif-labels
> -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv
> -fexcess-precision=standard -g -O2 -Wall -Werror -fPIC -I. -I.
> -I../../src/include -I/usr/include/x86_64-linux-gnu -D_GNU_SOURCE   -c -o
> verify_nbtree.o verify_nbtree.c
> verify_nbtree.c: In function ‘bt_check_every_level’:
> verify_nbtree.c:614:11: error: passing argument 6 of
> ‘table_index_build_scan’ from incompatible pointer type
> [-Werror=incompatible-pointer-types]
>bt_tuple_present_callback, (void *) state, scan);
>^
> In file included from verify_nbtree.c:29:0:
> ../../src/include/access/tableam.h:1499:1: note: expected
> ‘IndexBuildCallback {aka void (*)(struct RelationData *, struct
> ItemPointerData *, long unsigned int *, _Bool *, _Bool,  void *)}’ but
> argument is of type ‘void (*)(struct RelationData *, HeapTupleData *, Datum
> *, _Bool *, _Bool,  void *) {aka void (*)(struct RelationData *, struct
> HeapTupleData *, long unsigned int *, _Bool *, _Bool,  void *)}’
>  table_index_build_scan(Relation table_rel,
>  ^
> cc1: all warnings being treated as errors
> : recipe for target 'verify_nbtree.o' failed
> make[4]: *** [verify_nbtree.o] Error 1
>

Thanks for reporting, I did indeed missed out contrib. Please find attached
the v2 of the patch which includes the change required in contrib as well.
From 873711e0601d5035a302c9f2a4147250e902a28f Mon Sep 17 00:00:00 2001
From: Ashwin Agrawal 
Date: Wed, 25 Sep 2019 22:19:43 -0700
Subject: [PATCH v2] Remove IndexBuildCallback's dependency on HeapTuple

With IndexBuildCallback taking input as HeapTuple, all table AMs
irrespective of storing the tuples in HeapTuple form or not, are
forced to construct HeapTuple, to insert the tuple in Index. Since,
only thing required by the index callbacks is TID and not really the
full tuple, modify callback to only take ItemPointer.
---
 contrib/amcheck/verify_nbtree.c  |  6 +++---
 contrib/bloom/blinsert.c |  4 ++--
 src/backend/access/brin/brin.c   |  4 ++--
 src/backend/access/gin/gininsert.c   |  4 ++--
 src/backend/access/gist/gistbuild.c  |  6 +++---
 src/backend/access/hash/hash.c   |  8 
 src/backend/access/heap/heapam_handler.c | 11 +--
 src/backend/access/nbtree/nbtsort.c  |  8 
 src/backend/access/spgist/spginsert.c|  4 ++--
 src/include/access/tableam.h |  2 +-
 10 files changed, 28 insertions(+), 29 deletions(-)

diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c
index 05e7d678ed..3542545de5 100644
--- a/contrib/amcheck/verify_nbtree.c
+++ b/contrib/amcheck/verify_nbtree.c
@@ -140,7 +140,7 @@ static BTScanInsert bt_right_page_check_scankey(BtreeCheckState *state);
 static void bt_downlink_check(BtreeCheckState *state, BTScanInsert targetkey,
 			  BlockNumber childblock);
 static void bt_downlink_missing_check(BtreeCheckState *state);
-static void bt_tuple_present_callback(Relation index, HeapTuple htup,
+static void bt_tuple_present_callback(Relation index, ItemPointer tid,
 	  Datum *values, bool *isnull,
 	  bool tupleIsAlive, void *checkstate);
 static IndexTuple bt_normalize_tuple(BtreeCheckState *state,
@@ -1890,7 +1890,7 @@ bt_downlink_missing_check(BtreeCheckState *state)
  * also allows us to detect the corruption in many cases.
  */
 static void
-bt_tuple_present_callback(Relation index, HeapTuple htup, Datum *values,
+bt_tuple_present_callback(Relation index, ItemPointer tid, Datum *values,
 		  bool *isnull, bool tupleIsAlive, void *checkstate)
 {
 	BtreeCheckState *state = (BtreeCheckState *) checkstate;
@@ -1901,7 +1901,7 @@ bt_tuple_present_callback(Relation index, HeapTuple htup, Datum *values,
 
 	/* Generate a normalized index tuple for fingerprinting */
 	itup = index_form_tuple(RelationGetDescr(index), values, isnull);
-	itup->t_tid = htup->t_self;
+	itup->t_tid = *tid;
 	norm = bt_normalize_tuple(state, itup);
 
 	/* Probe Bloom filter -- tuple should be present */
diff --git a/contrib/bloom/blinsert.c b/contrib/bloom/blinsert.c
index 4b2186b8dd..66c6c1858d 100644
--- a/contrib/bloom/blinsert.c
+++ b/contrib/bloom/blinsert.c
@@ -73,7 +73,7 @@ initCachedPage(BloomBuildState *buildstate)
  * Per-tuple callback for table_index_build_scan.
  */
 static void
-bloomBuildCallback(Relation index, HeapTuple htup, Datum *values,
+bloomBuildCallback(Relation index, ItemPointer tid, Datum *values,
    bool *isnull, bool tupleIsAlive, void *state)
 {
 	BloomBuildState *buildstate = (BloomBuildState *) state;
@@ -82,7 +82,7 @@ bloomBuildCallback(Relation index, HeapTuple htup, Datum *values,
 
 	oldCtx 

Re: Don't codegen deform code for virtual tuples in expr eval for scan fetch

[Soumyadeep Chakraborty]

Hi Andres,

Thank you for your insight and the link offered just the context I needed!

On Wed, Sep 25, 2019 at 1:06 PM Andres Freund  wrote:

> > I'm doubtful this is worth the complexity - and not that we already have
> plenty other places with zero length blocks.

Agreed.

On Wed, Sep 25, 2019 at 1:06 PM Andres Freund  wrote:

> > WRT jit_optimize_above_cost not triggering, I think we need to replace
> the "basic, unoptimized" codegen path with one that does cheap
> optimizations only. The reason we don't do the full optimizations all
> the time is that they're expensive, but there's enough optimizations
> that are pretty cheap.  At some point we'll probably need our own
> optimization pipeline, but I don't want to maintain that right now
> (i.e. if some other people want to help with this aspect, cool)...

I would absolutely love to work on this!

I was thinking the same. Perhaps not only replace the default on the
compile path, but also introduce additional levels. These levels could then
be configured at a granularity lower than the -O0, -O1, .. In other words,
perhaps we could have levels with ad-hoc pass combinations. We could then
maybe have costs associated with each level. I think such a framework
would be ideal.

To summarize this into TODOs:
1. Tune default compilation path optimizations.
2. New costs per optimization level.
3. Capacity to define "levels" with ad-hoc pass combinations that are
costing
sensitive.

Is this what you meant by "optimization pipeline"?

--
Soumyadeep

Re: Don't codegen deform code for virtual tuples in expr eval for scan fetch

[Soumyadeep Chakraborty]

Hello Andres,

Thank you very much for reviewing my patch!

On Wed, Sep 25, 2019 at 1:02 PM Andres Freund  wrote:
> IOW, wherever ExecComputeSlotInfo() is called, we should only actually
> push the expression step, when ExecComputeSlotInfo does not determine
> that a) the slot is virtual, b) and fixed (i.e. guaranteed to always be
> the same type of slot).
>
> Does that make sense?

That is a great suggestion and I totally agree. I have attached a patch
that achieves this.

--
Soumyadeep


v1-0001-Skip-deform-expr-step-emission-for-virtual-tuples.patch
Description: Binary data

Re: partition routing layering in nodeModifyTable.c

[Amit Langote]

On Wed, Sep 4, 2019 at 10:45 AM Amit Langote  wrote:
> On Fri, Aug 9, 2019 at 10:51 AM Amit Langote  wrote:
> To avoid losing track of this, I've added this to November CF.
>
> https://commitfest.postgresql.org/25/2277/
>
> Struggled a bit to give a title to the entry though.

Noticed that one of the patches needed a rebase.

Attached updated patches.  Note that v8-0001 is v7-0001 unchanged that
Fujita-san posted on Aug 8.

Thanks,
Amit


v8-0001-Remove-dependency-on-estate-es_result_relation_in.patch
Description: Binary data


v8-0004-Refactor-transition-tuple-capture-code-a-bit.patch
Description: Binary data


v8-0002-Remove-es_result_relation_info.patch
Description: Binary data


v8-0003-Rearrange-partition-update-row-movement-code-a-bi.patch
Description: Binary data

Re: PostgreSQL12 and older versions of OpenSSL

[Michael Paquier]

On Tue, Sep 24, 2019 at 11:52:48PM +0200, Peter Eisentraut wrote:
> That's not actually what this file looks like in the upstream release.
> It looks like the packagers must have patched in the protocol codes for
> TLS 1.1 and 1.2 themselves.  Then they should also add the corresponding
> SSL_OP_NO_* flags.  AFAICT, these pairs of symbols are always added
> together in upstream commits.

Yes, they did so.  I see those three fields as of 6287fa5 from
upstream which is the release tag for 0.9.8j:
#define TLS1_VERSION0x0301
#define TLS1_VERSION_MAJOR  0x03
#define TLS1_VERSION_MINOR  0x01

However if you look at the top branch OpenSSL_0_9_8-stable (7474341),
then you would notice that ssl/tls1.h does something completely
different and defines TLS_MAX_VERSION.  So everything is in line to
say that the version of OpenSSL in SUSE labelled 0.9.8j is using
something compatible with the latest version of upstream 0.9.8zi.  I
think that we should just stick for simplicity with the top of their
branch instead of trying to be compatible with 0.9.8j because Postgres
12 has not been released yet, hence if one tries to compile Postgres
12 with OpenSSL 0.9.8j then they would get a compilation failure, and
we could just tell them to switch to the latest version of upstream
for 0.9.8.   That's something they should really do anyway to take
care of various security issues on this branch.  Well, if that happens
they should rather upgrade to at least 1.0.2 anyway :)

So I agree with the proposal to rely on the presence of
TLS_MAX_VERSION, and base our decision-making on that.
--
Michael


signature.asc
Description: PGP signature

Re: pgbench - allow to create partitioned tables

[Amit Kapila]

On Tue, Sep 24, 2019 at 6:59 PM Fabien COELHO  wrote:
>
> > For you or me, it might be okay as we have discussed this case, but it
> > won't be apparent to others.  This doesn't buy us much, so it is better
> > to keep this code consistent with other places that check for
> > partitions.
>
> Attached uses "partition_method != PART_NONE" consistently, plus an assert
> on "partitions > 0" for checking and for triggering the default method at
> the end of option processing.
>

Okay, I think making the check consistent is a step forward.  The
latest patch is not compiling for me.  You have used the wrong
variable name in below line:
+ /* Partition pgbench_accounts table */
+ if (partitions_method != PART_NONE && strcmp(ddl->table,
"pgbench_accounts") == 0)

Another point is:
+ else if (PQntuples(res) == 0)
+ {
+ /*
+ * This case is unlikely as pgbench already found "pgbench_branches"
+ * above to compute the scale.
+ */
+ fprintf(stderr,
+ "No pgbench_accounts table found in search_path. "
+ "Perhaps you need to do initialization (\"pgbench -i\") in database
\"%s\"\n", PQdb(con));

We don't recommend to start messages with a capital letter.  See
"Upper Case vs. Lower Case" section in docs [1].  It is not that we
have not used it anywhere else, but I think we should try to avoid it.

[1] - https://www.postgresql.org/docs/devel/error-style-guide.html

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

Re: A comment fix in xlogreader.c

[Michael Paquier]

On Thu, Sep 26, 2019 at 11:08:09AM +0900, Kyotaro Horiguchi wrote:
> While rechecking another patch, I found that 709d003fbd forgot to
> edit a comment mentioning three members removed from
> XLogReaderState.
>
@@ -103,8 +103,7 @@ XLogReaderAllocate(int wal_segment_size, const char *waldir,
 state->read_page = pagereadfunc;
 /* system_identifier initialized to zeroes above */
 state->private_data = private_data;
-/* ReadRecPtr and EndRecPtr initialized to zeroes above */
-/* readSegNo, readOff, readLen, readPageTLI initialized to zeroes above */
+/* ReadRecPtr, EndRecPtr and readLen initialized to zeroes above */
 state->errormsg_buf = palloc_extended(MAX_ERRORMSG_LEN + 1,
   MCXT_ALLOC_NO_OOM);
 if (!state->errormsg_buf)

I see.  readSegNo and readOff have been moved to WALOpenSegment and
replaced by new, equivalent fields, still all those three fields are
still initialized for the palloc_extended() call to allocate
XLogReaderState, while the two others are now part of
WALOpenSegmentInit().  Your change is correct, so applied.
--
Michael


signature.asc
Description: PGP signature

A comment fix in xlogreader.c

[Kyotaro Horiguchi]

While rechecking another patch, I found that 709d003fbd forgot to
edit a comment mentioning three members removed from
XLogReaderState.

See the attached.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 27c27303d6..c8b0d2303d 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -103,8 +103,7 @@ XLogReaderAllocate(int wal_segment_size, const char *waldir,
 	state->read_page = pagereadfunc;
 	/* system_identifier initialized to zeroes above */
 	state->private_data = private_data;
-	/* ReadRecPtr and EndRecPtr initialized to zeroes above */
-	/* readSegNo, readOff, readLen, readPageTLI initialized to zeroes above */
+	/* ReadRecPtr, EndRecPtr and readLen initialized to zeroes above */
 	state->errormsg_buf = palloc_extended(MAX_ERRORMSG_LEN + 1,
 		  MCXT_ALLOC_NO_OOM);
 	if (!state->errormsg_buf)

Re: Re: row filtering for logical replication

[movead...@highgo.ca]

>Which regression?
Apply the 0001.patch~0005.patch and then do a 'make check', then there be a
failed item. And when you apply the 0006.patch, the failed item disappeared.

>There should be an error because you don't have a PK or REPLICA IDENTITY.
No. I have done the 'ALTER TABLE cities  REPLICA IDENTITY FULL'.

>Even if you create a PK or REPLICA IDENTITY, it won't turn this UPDATE
>into a INSERT and send it to the other node (indeed UPDATE will be
>sent however there isn't a tuple to update). Also, filter columns must
>be in PK or REPLICA IDENTITY. I explain this in documentation.
You should considered the Result2:
 On publication:
  insert into cities values('t1',1,135);
  update cities set altitude=300 where altitude=135;
  postgres=# table cities ;
   name | population | altitude 
  --++--
   t1   |123 |  134
   t1   |  1 |  300
  (2 rows)
  
  On subscription:
  ostgres=# table cities ;
   name | population | altitude 
  --++--
   t1   |  1 |  135

The tuple ('t1',1,135) appeared in both publication and subscription,
but after an update on publication, the tuple is disappeared on 
publication and change nothing on subscription.

The same with Result1, they puzzled me today and I think they will
puzzle the users in the future. It should have a more wonderful design,
for example, a log to notify users that there be a problem during replication
at least.

---
Highgo Software (Canada/China/Pakistan) 
URL : www.highgo.ca 
EMAIL: mailto:movead(dot)li(at)highgo(dot)ca
 
 
 
 

Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

[Amit Langote]

On Thu, Sep 26, 2019 at 5:15 AM Alvaro Herrera  wrote:
> Travis complains that this patch adds a new compile warning.  Please
> fix.

Thanks, updated patch attached.

Regards,
Amit


v6-0001-Use-root-parent-s-permissions-when-reading-child-.patch
Description: Binary data

Re: row filtering for logical replication

[Euler Taveira]

Em qua, 25 de set de 2019 às 08:08, Euler Taveira
 escreveu:
>
> I'll send a patchset later today.
>
... and it is attached.


-- 
   Euler Taveira   Timbira -
http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
From b5d4d1369dbb4e7ec20182507dc5ae920dd8d2e9 Mon Sep 17 00:00:00 2001
From: Euler Taveira 
Date: Fri, 9 Mar 2018 18:39:22 +
Subject: [PATCH 1/8] Remove unused atttypmod column from initial table
 synchronization

 Since commit 7c4f52409a8c7d85ed169bbbc1f6092274d03920, atttypmod was
 added but not used. The removal is safe because COPY from publisher
 does not need such information.
---
 src/backend/replication/logical/tablesync.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 7881079..0a565dd 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -647,7 +647,7 @@ fetch_remote_table_info(char *nspname, char *relname,
 	StringInfoData cmd;
 	TupleTableSlot *slot;
 	Oid			tableRow[2] = {OIDOID, CHAROID};
-	Oid			attrRow[4] = {TEXTOID, OIDOID, INT4OID, BOOLOID};
+	Oid			attrRow[3] = {TEXTOID, OIDOID, BOOLOID};
 	bool		isnull;
 	int			natt;
 
@@ -691,7 +691,6 @@ fetch_remote_table_info(char *nspname, char *relname,
 	appendStringInfo(,
 	 "SELECT a.attname,"
 	 "   a.atttypid,"
-	 "   a.atttypmod,"
 	 "   a.attnum = ANY(i.indkey)"
 	 "  FROM pg_catalog.pg_attribute a"
 	 "  LEFT JOIN pg_catalog.pg_index i"
@@ -703,7 +702,7 @@ fetch_remote_table_info(char *nspname, char *relname,
 	 lrel->remoteid,
 	 (walrcv_server_version(wrconn) >= 12 ? "AND a.attgenerated = ''" : ""),
 	 lrel->remoteid);
-	res = walrcv_exec(wrconn, cmd.data, 4, attrRow);
+	res = walrcv_exec(wrconn, cmd.data, 3, attrRow);
 
 	if (res->status != WALRCV_OK_TUPLES)
 		ereport(ERROR,
@@ -724,7 +723,7 @@ fetch_remote_table_info(char *nspname, char *relname,
 		Assert(!isnull);
 		lrel->atttyps[natt] = DatumGetObjectId(slot_getattr(slot, 2, ));
 		Assert(!isnull);
-		if (DatumGetBool(slot_getattr(slot, 4, )))
+		if (DatumGetBool(slot_getattr(slot, 3, )))
 			lrel->attkeys = bms_add_member(lrel->attkeys, natt);
 
 		/* Should never happen. */
-- 
2.7.4

From 406b2dbe4df63a94364e548a67d085e255ea2644 Mon Sep 17 00:00:00 2001
From: Euler Taveira 
Date: Fri, 9 Mar 2018 17:37:36 +
Subject: [PATCH 2/8] Store number of tuples in WalRcvExecResult

It seems to be a useful information while allocating memory for queries
that returns more than one row. It reduces memory allocation
for initial table synchronization.
---
 src/backend/replication/libpqwalreceiver/libpqwalreceiver.c | 5 +++--
 src/backend/replication/logical/tablesync.c | 5 ++---
 src/include/replication/walreceiver.h   | 1 +
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 6eba08a..343550a 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -878,6 +878,7 @@ libpqrcv_processTuples(PGresult *pgres, WalRcvExecResult *walres,
  errdetail("Expected %d fields, got %d fields.",
 		   nRetTypes, nfields)));
 
+	walres->ntuples = PQntuples(pgres);
 	walres->tuplestore = tuplestore_begin_heap(true, false, work_mem);
 
 	/* Create tuple descriptor corresponding to expected result. */
@@ -888,7 +889,7 @@ libpqrcv_processTuples(PGresult *pgres, WalRcvExecResult *walres,
 	attinmeta = TupleDescGetAttInMetadata(walres->tupledesc);
 
 	/* No point in doing more here if there were no tuples returned. */
-	if (PQntuples(pgres) == 0)
+	if (walres->ntuples == 0)
 		return;
 
 	/* Create temporary context for local allocations. */
@@ -897,7 +898,7 @@ libpqrcv_processTuples(PGresult *pgres, WalRcvExecResult *walres,
 	   ALLOCSET_DEFAULT_SIZES);
 
 	/* Process returned rows. */
-	for (tupn = 0; tupn < PQntuples(pgres); tupn++)
+	for (tupn = 0; tupn < walres->ntuples; tupn++)
 	{
 		char	   *cstrs[MaxTupleAttributeNumber];
 
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 0a565dd..42db4ad 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -709,9 +709,8 @@ fetch_remote_table_info(char *nspname, char *relname,
 (errmsg("could not fetch table info for table \"%s.%s\": %s",
 		nspname, relname, res->err)));
 
-	/* We don't know the number of rows coming, so allocate enough space. */
-	lrel->attnames = palloc0(MaxTupleAttributeNumber * sizeof(char *));
-	lrel->atttyps = palloc0(MaxTupleAttributeNumber * sizeof(Oid));
+	lrel->attnames = palloc0(res->ntuples * sizeof(char *));
+	lrel->atttyps = 

Re: add a MAC check for TRUNCATE

[Tom Lane]

Alvaro Herrera  writes:
> On 2019-Sep-25, Yuli Khodorkovskiy wrote:
>> Since all existing DAC checks should have MAC, should these patches be
>> considered a bug fix and therefore back patched?

> I don't know the answer to that.  My impression from earlier discussion
> is that this was seen as a non-backpatchable change, but I defer to Joe
> on that as committer.  If it were up to me, the ultimate question would
> be: would such a change adversely affect existing running systems?

I don't see how the addition of a new permissions check could sanely
be back-patched unless it were to default to "allow", which seems like
an odd choice.

regards, tom lane

Re: add a MAC check for TRUNCATE

[Alvaro Herrera]

On 2019-Sep-25, Yuli Khodorkovskiy wrote:

> Hi Alvaro,
> 
> I have attached the updated patches which should rebase.

Great, thanks.

> Since all existing DAC checks should have MAC, should these patches be
> considered a bug fix and therefore back patched?

I don't know the answer to that.  My impression from earlier discussion
is that this was seen as a non-backpatchable change, but I defer to Joe
on that as committer.  If it were up to me, the ultimate question would
be: would such a change adversely affect existing running systems?

Thanks,

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

Re: add a MAC check for TRUNCATE

[Yuli Khodorkovskiy]

On Wed, Sep 25, 2019 at 3:57 PM Alvaro Herrera  wrote:
>
> Hello
>
> On 2019-Sep-09, Yuli Khodorkovskiy wrote:
>
> > I have included an updated version of the sepgql patch. The
> > Truncate-Hook patch is unchanged from the last version.
>
> This patch no longer applies.  Can you please rebase?

Hi Alvaro,

I have attached the updated patches which should rebase.

Since all existing DAC checks should have MAC, should these patches be
considered a bug fix and therefore back patched?

Thank you


Truncate-Hook.patch
Description: Binary data


v3-Sepgsql-Truncate.patch
Description: Binary data

Re: pglz performance

[Petr Jelinek]

Hi,

On 05/08/2019 09:26, Andres Freund wrote:

Hi,

On 2019-08-05 00:19:28 +0200, Petr Jelinek wrote:

It carries that information inside the compressed value, like I said in the
other reply, that's why the extra byte.


I'm not convinced that that is a good plan - imo the reference to the
compressed data should carry that information.

I.e. in the case of toast, at least toast pointers should hold enough
information to determine the compression algorithm. And in the case of
WAL, the WAL record should contain that.


Point taken.



For external datums I suggest encoding the compression method as a
distinct VARTAG_ONDISK_COMPRESSED, and then have that include the
compression method as a field.


So the new reads/writes will use this and reads of old format won't 
change? Sounds fine.




For in-line compressed values (so VARATT_IS_4B_C), doing something
roughly like you did, indicating the type of metadata following using
the high bit sounds reasonable. But I think I'd make it so that if the
highbit is set, the struct is instead entirely different, keeping a full
4 byte byte length, and including the compression type header inside the
struct. Perhaps I'd combine the compression type with the high-bit-set
part?  So when the high bit is set, it'd be something like

{
 int32  vl_len_;/* varlena header (do not touch 
directly!) */

 /*
  * Actually only 7 bit, the high bit determines whether this
  * is the old compression header (if unset), or this type of header
  * (if set).
  */
 uint8 type;

 /*
  * Stored as uint8[4], to avoid unnecessary alignment padding.
  */
 uint8[4] length;

 char va_data[FLEXIBLE_ARRAY_MEMBER];
}



Won't this break BW compatibility on big-endian (if I understand 
corretly what you are trying to achieve here)?



I think it's worth spending some effort trying to get this right - we'll
be bound by design choices for a while.



Sure, however I am not in business of redesigning TOAST from scratch 
right now, even if I do agree that the current header is far from ideal.


--
Petr Jelinek
2ndQuadrant - PostgreSQL Solutions for the Enterprise
https://www.2ndQuadrant.com/

Re: DROP SUBSCRIPTION with no slot

[Petr Jelinek]

Hi,

On 25/09/2019 01:07, Jeff Janes wrote:
On Tue, Sep 24, 2019 at 5:25 PM Peter Eisentraut 
> wrote:


On 2019-09-24 16:31, Jeff Janes wrote:
 > I recently had to cut loose (pg_drop_replication_slot) a logical
replica
 > that couldn't keep up and so was threatening to bring down the
master.
 >
 > In mopping up on the replica side, I couldn't just drop the
 > subscription, because it couldn't drop the nonexistent slot on the
 > master and so refused to work.  So I had to do a silly little dance
 > where I first disable the subscription, then ALTER SUBSCRIPTION
... SET
 > (slot_name = NONE), then drop it.
 >
 > Wanting to clean up after itself is admirable, but if there is
nothing
 > to clean up, why should that be an error condition?

The alternatives seem quite error prone to me.  Better be explicit.


If you can connect to the master, and see that the slot already fails to 
exist, what is error prone about that?


If someone goes to the effort of setting up a different master, 
configures it to receive replica connections, and alters the 
subscription CONNECTION parameter on the replica to point to that 
poisoned master, will an error on the DROP SUBSCRIPTION really force 
them to see the error of their ways, or will they just succeed at 
explicitly doing the silly dance and finalize the process of shooting 
themselves in the foot via a roundabout mechanism? 


All that needs to happen to get into this situation is to have 
replication go through haproxy or some other loadbalancer or dns name 
that points to different server after failover. So user really does not 
have to touch the subscription


We should at least offer HINT though.

However, I'd be in favor of removing this restriction once the patch 
which limits how much wal a slot can retain gets in.


--
Petr Jelinek
2ndQuadrant - PostgreSQL Solutions for the Enterprise
https://www.2ndQuadrant.com/

Re: Speed up transaction completion faster after many relations are accessed in a transaction

[Alvaro Herrera]

On 2019-Sep-03, Tsunakawa, Takayuki wrote:

> From: Alvaro Herrera [mailto:alvhe...@2ndquadrant.com]
> > Hmm ... is this patch rejected, or is somebody still trying to get it to
> > committable state?  David, you're listed as committer.
> 
> I don't think it's rejected.  It would be a pity (mottainai) to refuse
> this, because it provides significant speedup despite its simple
> modification.

I don't necessarily disagree with your argumentation, but Travis is
complaining thusly:

gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement 
-Werror=vla -Wendif-labels -Wmissing-format-attribute -Wformat-security 
-fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -O2 -Wall -Werror 
-I../../../../src/include -I/usr/include/x86_64-linux-gnu -D_GNU_SOURCE   -c -o 
lock.o lock.c
1840lock.c:486:1: error: conflicting types for ‘TryShrinkLocalLockHash’
1841 TryShrinkLocalLockHash(long numLocksHeld)
1842 ^
1843lock.c:351:20: note: previous declaration of ‘TryShrinkLocalLockHash’ was 
here
1844 static inline void TryShrinkLocalLockHash(void);
1845^
1846: recipe for target 'lock.o' failed

Please fix.

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

Re: benchmarking Flex practices

[Tom Lane]

Alvaro Herrera  writes:
> ... it seems this patch needs attention, but I'm not sure from whom.
> The tests don't pass whenever the server encoding is not UTF8, so I
> suppose we should either have an alternate expected output file to
> account for that, or the tests should be removed.  But anyway the code
> needs to be reviewed.

Yeah, I'm overdue to review it, but other things have taken precedence.

The unportable test is not a problem at this point, since the patch
isn't finished anyway.  I'm not sure yet whether it'd be worth
preserving that test case in the final version.

regards, tom lane

Re: shared-memory based stats collector

[Alvaro Herrera]

On 2019-Sep-10, Kyotaro Horiguchi wrote:

> At Tue, 3 Sep 2019 18:28:05 -0400, Alvaro Herrera  
> wrote in <20190903222805.GA13932@alvherre.pgsql>
> > > Found a bug in initialization. StatsShememInit() was placed at a
> > > wrong place and stats code on child processes accessed
> > > uninitialized pointer. It is a leftover from the previous shape
> > > where dsm was activated on postmaster.
> > 
> > This doesn't apply anymore.  Can you please rebase?
> 
> Thanks! I forgot to post rebased version after doing. Here it is.
> 
> - (Re)Rebased to the current master.
> - Passed all tests for me.

This seems to have very trivial conflicts -- please rebase again?

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

Re: Copy data to DSA area

[Alvaro Herrera]

Should now this be closed as Returned with Feedback, or are we really
waiting on an updated patch in the short term?

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

Re: SQL/JSON: functions

[Alvaro Herrera]

This has recently been broken -- please rebase.  (JSON_TABLE likewise).

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

Re: Resume vacuum and autovacuum from interruption and cancellation

[Alvaro Herrera]

Apparently this patch now has a duplicate OID.  Please do use random
OIDs >8000 as suggested by the unused_oids script.

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

Re: heapam_index_build_range_scan's anyvisible

[Alvaro Herrera]

On 2019-Jul-30, Ashwin Agrawal wrote:

> On Tue, Jul 16, 2019 at 10:22 AM Andres Freund  wrote:

> > Looks good to me. Planning to apply this unless somebody wants to argue
> > against it soon?
> 
> Andres, I didn't yet register this for next commitfest. If its going in
> soon anyways will not do it otherwise let me know and I will add it to the
> list.

Sounds OK ... except that Travis points out that Ashwin forgot to patch contrib:

make[4]: Entering directory 
'/home/travis/build/postgresql-cfbot/postgresql/contrib/amcheck'
gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement 
-Werror=vla -Wendif-labels -Wmissing-format-attribute -Wformat-security 
-fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -O2 -Wall -Werror 
-fPIC -I. -I. -I../../src/include -I/usr/include/x86_64-linux-gnu -D_GNU_SOURCE 
  -c -o verify_nbtree.o verify_nbtree.c
verify_nbtree.c: In function ‘bt_check_every_level’:
verify_nbtree.c:614:11: error: passing argument 6 of ‘table_index_build_scan’ 
from incompatible pointer type [-Werror=incompatible-pointer-types]
   bt_tuple_present_callback, (void *) state, scan);
   ^
In file included from verify_nbtree.c:29:0:
../../src/include/access/tableam.h:1499:1: note: expected ‘IndexBuildCallback 
{aka void (*)(struct RelationData *, struct ItemPointerData *, long unsigned 
int *, _Bool *, _Bool,  void *)}’ but argument is of type ‘void (*)(struct 
RelationData *, HeapTupleData *, Datum *, _Bool *, _Bool,  void *) {aka void 
(*)(struct RelationData *, struct HeapTupleData *, long unsigned int *, _Bool 
*, _Bool,  void *)}’
 table_index_build_scan(Relation table_rel,
 ^
cc1: all warnings being treated as errors
: recipe for target 'verify_nbtree.o' failed
make[4]: *** [verify_nbtree.o] Error 1

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

Re: Refactoring of connection with password prompt loop for frontends

[Alvaro Herrera]

This patch has been absolutely overlooked by reviewers.  Euler, you're
registered as a reviewer for it.  Will you submit a review soon?  :-)

It does not currently apply, so if we get a rebase, that'd be good too.

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

Re: add a MAC check for TRUNCATE

[Joe Conway]

On 9/25/19 3:56 PM, Alvaro Herrera wrote:
> Hello
> 
> On 2019-Sep-09, Yuli Khodorkovskiy wrote:
> 
>> I have included an updated version of the sepgql patch. The
>> Truncate-Hook patch is unchanged from the last version.
> 
> This patch no longer applies.  Can you please rebase?
> 
> Joe, do you plan on being committer for this patch?  There seems to be
> substantial agreement on it.


I should be able to do that.

Joe

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

Re: benchmarking Flex practices

[Alvaro Herrera]

... it seems this patch needs attention, but I'm not sure from whom.
The tests don't pass whenever the server encoding is not UTF8, so I
suppose we should either have an alternate expected output file to
account for that, or the tests should be removed.  But anyway the code
needs to be reviewed.

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

Re: [proposal] de-TOAST'ing using a iterator

[Paul Ramsey]

> On Sep 23, 2019, at 9:45 AM, Alvaro Herrera  wrote:
> 
> Paul Ramsey, do you have opinions to share about this patch?  I think
> PostGIS might benefit from it.  Thread starts here:

I like the idea a great deal, but actually PostGIS is probably neutral on it: 
we generally want to retrieve things off the front of our serializations (the 
metadata header) rather than search through them for things in the middle. So 
the improvements to Pg12 cover all of our use cases. Haven’t had time to do any 
performance checks on it yet.

ATB,

P.

Re: Optimize partial TOAST decompression

[Alvaro Herrera]

Hello, can you please update this patch?

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

Re: NOT IN subquery optimization

[Alvaro Herrera]

On 2019-Aug-02, Thomas Munro wrote:

> Hi Zheng, Jim,
> 
> With my Commitfest doozer hat on, I have moved this entry to the
> September 'fest.  I noticed in passing that it needs to be adjusted
> for the new pg_list.h API.  It'd be good to get some feedback from
> reviewers on these two competing proposals:
> 
> https://commitfest.postgresql.org/24/2020/
> https://commitfest.postgresql.org/24/2023/

Hello,

Is this patch dead?

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

Re: [PATCH] ltree, lquery, and ltxtquery binary protocol support

[Alvaro Herrera]

Hello, can you please post an updated version of this patch?  Note that
Travis has a small complaint:

gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement 
-Werror=vla -Wendif-labels -Wmissing-format-attribute -Wformat-security 
-fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -O2 -Wall -Werror 
-fPIC -DLOWER_NODE -I. -I. -I../../src/include -I/usr/include/x86_64-linux-gnu 
-D_GNU_SOURCE   -c -o crc32.o crc32.c
crc32.c: In function ‘ltree_crc32_sz’:
crc32.c:26:12: error: initialization discards ‘const’ qualifier from pointer 
target type [-Werror=discarded-qualifiers]
  char*p = buf;
^
cc1: all warnings being treated as errors
: recipe for target 'crc32.o' failed
make[4]: *** [crc32.o] Error 1
make[4]: Leaving directory 
'/home/travis/build/postgresql-cfbot/postgresql/contrib/ltree'

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

Re: Global temporary tables

[Alvaro Herrera]

This broke recently.  Can you please rebase?

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

Re: Global shared meta cache

[Alvaro Herrera]

The last patch we got here (a prototype) was almost a year ago.  There
was substantial discussion about it, but no new version of the patch has
been posted.  Are we getting a proper patch soon, or did we give up on
the approach entirely?

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

Re: Two pg_rewind patches (auto generate recovery conf and ensure clean shutdown)

[a . kondratov]

On 2019-09-25 20:48, Alvaro Herrera wrote:

CC Alexey for reasons that become clear below.

Another thing in 0002 is that you're adding a "-R" switch to pg_rewind,
but we have another patch in the commitfest using the same switch for a
different purpose.  Maybe you guys need to get to an agreement over who
uses the letter :-)



Thank you for mentioning me. I've been monitoring silently this thread 
and was ready to modify my patch if this one will proceed faster. It 
seems like it's time :)


On 2019-09-25 22:26, Laurenz Albe wrote:


I believe that -R should be reserved for creating recovery.conf,
similar to pg_basebackup.

Everything else would be confusing.

I've been missing pg_rewind -R!



Yes, -R is already used in pg_basebackup for the same functionality, so 
it seems natural to use it here as well for consistency.


I will review options naming in my own patch and update it accordingly. 
Maybe -w/-W or -a/-A options will be good, since it's about WALs 
retrieval from archive.



Regards
--
Alexey

P.S. Just noticed that in v12 fullname of -R option in pg_basebackup is 
still --write-recovery-conf, which is good for a backward compatibility, 
but looks a little bit awkward, since recovery.conf doesn't exist 
already, doesn't it? However, one may read it as 
'write-recovery-configuration', then it seems fine.

Standby accepts recovery_target_timeline setting?

[David Steele]

While testing against PG12 I noticed the documentation states that
recovery targets are not valid when standby.signal is present.

But surely the exception is recovery_target_timeline?  My testing
confirms that this works just as in prior versions with standy_mode=on.

Documentation patch is attached.

Regards,
-- 
-David
da...@pgmasters.net
From 5c6cb5e75dcd122268d2ac0dcb99a2c2bbb0e9e5 Mon Sep 17 00:00:00 2001
From: David Steele 
Date: Wed, 25 Sep 2019 16:11:02 -0400
Subject: [PATCH] Add timeline as valid recovery target in standby.signal
 documentation.

The documentation states that no target settings will be used when
standby.signal is present, but this is not quite the case since
recovery_target_timeline is a valid recovery target for a standby.

Update the documentation with this exception.
---
 doc/src/sgml/config.sgml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 6612f95f9f..4784b4b18e 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3156,7 +3156,9 @@ include_dir 'conf.d'
  use parameters in both  and .  Parameters from
-  will not be used.
+  will not be used,
+ with the exception of 
+ which allows a timeline to be selected for recovery.
 
 
 
-- 
2.20.1 (Apple Git-117)

Re: [HACKERS] [PATCH] Generic type subscripting

[Alvaro Herrera]

This broke recently.  Can you please rebase again?

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

Re: [HACKERS] Custom compression methods

[Alvaro Herrera]

The compile of this one has been broken for a long time.  Is there a
rebase happening?

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

Re: Change atoi to strtol in same place

[Alvaro Herrera]

... can we have a new patch?  Not only because there seems to have been
some discussion points that have gone unaddressed (?), but also because
Appveyor complains really badly about this one.
https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.58672

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

Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

[Alvaro Herrera]

Travis complains that this patch adds a new compile warning.  Please
fix.

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

Re: Built-in connection pooler

[Alvaro Herrera]

Travis complains that the SGML docs are broken.  Please fix.

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

Re: Bloom index cost model seems to be wrong

[Alvaro Herrera]

It's not clear to me what the next action should be on this patch.  I
think Jeff got some feedback from Tom, but was that enough to expect a
new version to be posted?  That was in February; should we now (in late
September) close this as Returned with Feedback?

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

Re: WIP: BRIN multi-range indexes

[Alvaro Herrera]

This patch fails to apply (or the opclass params one, maybe).  Please
update.

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

Re: Don't codegen deform code for virtual tuples in expr eval for scan fetch

[Andres Freund]

Hi,

On 2019-09-20 22:19:46 -0700, Soumyadeep Chakraborty wrote:
> In my previous patch 0001, the resulting opblock consisted of a single
> br instruction to it's successor opblock. Such a block represents
> unnecessary overhead. Even though such a block would be optimized
> away, what if optimization is not performed (perhaps due to
> jit_optimize_above_cost)? Perhaps we could be more aggressive. We
> could maybe remove the opblock altogether. However, such a solution
> is not without complexity.

I'm doubtful this is worth the complexity - and not that we already have
plenty other places with zero length blocks.

WRT jit_optimize_above_cost not triggering, I think we need to replace
the "basic, unoptimized" codegen path with one that does cheap
optimizations only. The reason we don't do the full optimizations all
the time is that they're expensive, but there's enough optimizations
that are pretty cheap.  At some point we'll probably need our own
optimization pipeline, but I don't want to maintain that right now
(i.e. if some other people want to help with this aspect, cool)...

See also: 
https://www.postgresql.org/message-id/20190904152438.pv4vdk3ctuvvnxh3%40alap3.anarazel.de

Greetings,

Andres Freund

Re: [HACKERS] Cached plans and statement generalization

[Alvaro Herrera]

This patch fails the "rules" tests. Please fix.


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

Re: [Patch] Add a reset_computed_values function in pg_stat_statements

[Alvaro Herrera]

This patch seems to be failing the contrib build.  Please fix.

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

Re: Add FOREIGN to ALTER TABLE in pg_dump

[Alvaro Herrera]

On 2019-Jul-12, Luis Carril wrote:

> Hello,
> pg_dump creates plain ALTER TABLE statements even if the table is a 
> foreign table, which for someone reading the dump is confusing.
> This also made a difference when applying the dump if there is any plugin 
> installed that hooks on ProcessUtility, because the plugin could react 
> differently to ALTER TABLE than to ALTER FOREIGN TABLE.  Opinions?

I think such a hook would be bogus, because it would miss anything done
by a user manually.

I don't disagree with adding FOREIGN, though.

Your patch is failing the pg_dump TAP tests.  Please use
configure --enable-tap-tests, fix the problems, then resubmit.

> An unrelated question: if I apply pgindent to a file (in this case 
> pg_dump.c) and get a bunch of changes on the indentation that are not related 
> to my patch, which is the accepted policy? A different patch first with only 
> the indentation?  Maybe, am I using pgindent wrong?

We don't typically accept pgindent-only changes at random points in
the devel cycle.

I would suggest to run pgindent over the file and "git add -p" only the
changes that are relevant to your patch, discard the rest.
(Alternative: run pgindent, commit that, then apply your patch, pgindent
again and "git commit --amend", then "git rebase -i" and discard the
first pgindent commit.  Your patch ends up pgindent-correct without
disturbing the rest of the file/tree).

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

Re: Don't codegen deform code for virtual tuples in expr eval for scan fetch

[Andres Freund]

Hi,

Thanks for working on this!

On 2019-09-17 23:54:51 -0700, Soumyadeep Chakraborty wrote:
> This is to address a TODO I found in the JIT expression evaluation
> code (opcode =
> EEOP_INNER_FETCHSOME/EEOP_OUTER_FETCHSOME/EEOP_SCAN_FETCHSOME):
> 
> * TODO: skip nvalid check if slot is fixed and known to
> * be a virtual slot.

I now think this isn't actually the right approach. Instead of doing
this optimization just for JIT compilation, we should do it while
generating the ExprState itself. That way we don't just accelerate JITed
programs, but also normal interpreted execution.

IOW, wherever ExecComputeSlotInfo() is called, we should only actually
push the expression step, when ExecComputeSlotInfo does not determine
that a) the slot is virtual, b) and fixed (i.e. guaranteed to always be
the same type of slot).

Does that make sense?

Greetings,

Andres Freund

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

[Alvaro Herrera]

This patch no longer applies.  Can you please rebase?

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

Re: add a MAC check for TRUNCATE

[Alvaro Herrera]

Hello

On 2019-Sep-09, Yuli Khodorkovskiy wrote:

> I have included an updated version of the sepgql patch. The
> Truncate-Hook patch is unchanged from the last version.

This patch no longer applies.  Can you please rebase?

Joe, do you plan on being committer for this patch?  There seems to be
substantial agreement on it.

Thanks

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

Re: Support for jsonpath .datetime() method

[Alexander Korotkov]

On Mon, Sep 23, 2019 at 10:05 PM Alexander Korotkov
 wrote:
> I've reordered the patchset.  I moved the most debatable patch, which
> introduces  and RR and changes parsing of YYY, YY and Y to the
> end.  I think we have enough of time in this release cycle to decide
> whether we want this.
>
> Patches 0001-0005 looks quite mature for me.  I'm going to push this
> if no objections.  After pushing them, I'm going to start discussion
> related to RR, YY and friends in separate thread.

Pushed.  Remaining patch is attached.  I'm going to start the separate
thread with its detailed explanation.

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


0001-Introduce--and-RR-revise-YYY-YY-and-Y-datetime-9.patch
Description: Binary data

Re: Control your disk usage in PG: Introduction to Disk Quota Extension

[Alvaro Herrera]

This patch no longer applies. Can you please rebase?

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

Re: Two pg_rewind patches (auto generate recovery conf and ensure clean shutdown)

[Laurenz Albe]

On Wed, 2019-09-25 at 14:48 -0300, Alvaro Herrera wrote:
> Another thing in 0002 is that you're adding a "-R" switch to
> pg_rewind, but we have another patch in the commitfest using the same
> switch for a different purpose.  Maybe you guys need to get to an
> agreement over who uses the letter :-)  Also, it would be super
> helpful if you review Alexey's patch:
> https://commitfest.postgresql.org/24/1849/

I believe that -R should be reserved for creating recovery.conf,
similar to pg_basebackup.

Everything else would be confusing.

I've been missing pg_rewind -R!

Yours,
Laurenz Albe

Re: [HACKERS] [WIP] Effective storage of duplicates in B-tree index.

[Peter Geoghegan]

On Wed, Sep 25, 2019 at 8:05 AM Anastasia Lubennikova
 wrote:
> Attached is v18. In this version bt_dedup_one_page() is refactored so that:
> - no temp page is used, all updates are applied to the original page.
> - each posting tuple wal logged separately.
> This also allowed to simplify btree_xlog_dedup significantly.

This looks great! Even if it isn't faster than using a temp page
buffer, the flexibility seems like an important advantage. We can do
things like have the _bt_dedup_one_page() caller hint that
deduplication should start at a particular offset number. If that
doesn't work out by the time the end of the page is reached (whatever
"works out" may mean), then we can just start at the beginning of the
page, and work through the items we skipped over initially.

> > We still haven't added an "off" switch to deduplication, which seems
> > necessary. I suppose that this should look like GIN's "fastupdate"
> > storage parameter.

> Why is it necessary to save this information somewhere but rel->rd_options,
> while we can easily access this field from _bt_findinsertloc() and
> _bt_load().

Maybe, but we also need to access a flag that says it's safe to use
deduplication. Obviously deduplication is not safe for datatypes like
numeric and text with a nondeterministic collation. The "is
deduplication safe for this index?" mechanism will probably work by
doing several catalog lookups. This doesn't seem like something we
want to do very often, especially with a buffer lock held -- ideally
it will be somewhere that's convenient to access.

Do we want to do that separately, and have a storage parameter that
says "I would like to use deduplication in principle, if it's safe"?
Or, do we store both pieces of information together, and forbid
setting the storage parameter to on when it's known to be unsafe for
the underlying opclasses used by the index? I don't know.

I think that you can start working on this without knowing exactly how
we'll do those catalog lookups. What you come up with has to work with
that before the patch can be committed, though.

-- 
Peter Geoghegan

Re: [PATCH][PROPOSAL] Add enum releation option type

[Alvaro Herrera]

On 2019-Sep-17, Alvaro Herrera wrote:

> I decided I didn't dislike that patch all that much anyway, so I cleaned
> it up a little bit and here's v8.
> 
> The add_enum_reloption stuff is still broken.  Please fix it and
> resubmit.  I'm marking this Waiting on Author now.

I finished this and pushed.

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

Re: Proposal for syntax to support creation of partition tables when creating parent table

[Ahsan Hadi]

On Wed, Sep 25, 2019 at 8:53 PM Tom Lane  wrote:

> Muhammad Usama  writes:
> > I want to propose an extension to CREATE TABLE syntax to allow the
> creation
> > of partition tables along with its parent table using a single statement.
>
> TBH, I think this isn't a particularly good idea.  It seems very
> reminiscent of the variant of CREATE SCHEMA that lets you create
> a bunch of contained objects along with the schema.  That variant
> is a mess to support and AFAIK it's practically unused in the
> real world.  (If it were used, we'd get requests to support more
> than the small number of object types that the CREATE SCHEMA
> grammar currently allows.)
>

IMO creating auto-partitions shouldn't be viewed as creating bunch of
schema objects with CREATE SCHEMA command. Most of the other RDBMS
solutions support the table partition syntax where parent partition table
is specified with partitions and sub-partitions in same SQL statement. As I
understand the proposal is not changing the syntax of creating partitions,
it is providing the ease of creating parent partition table along with its
partitions in same statement. I think it does make it easier when you are
creating a big partition table with lots of partitions and sub-partitions.

The would also benefit users migrating to postgres from Oracle or mysql etc
where similar syntax is supported.

And if not more I think it is a tick in the box with minimal code change.


>
> As Fabien noted, there's been some related discussion about this
> area, but nobody was advocating a solution of this particular shape.
>

The thread that Usama mentioned in his email is creating auto-partitions
just for HASH partitions, this is trying to do similar for all types of
partitions.


> regards, tom lane
>
>
>

-- 
Highgo Software (Canada/China/Pakistan)
URL : http://www.highgo.ca
ADDR: 10318 WHALLEY BLVD, Surrey, BC
EMAIL: mailto: ahsan.h...@highgo.ca

Re: DROP SUBSCRIPTION with no slot

[Isaac Morland]

On Wed, 25 Sep 2019 at 13:55, Žiga Kranjec  wrote:

>
> Ah. I missed that bit in the documentation!
>
> Perhaps a publication should remember, whether it actually created a
> replication slot and only try to remove it, if it did. Although that
> probably wouldn't help much in your case.
>

What about issuing a NOTICE if the slot doesn't exist? I'm thinking if it
is able to connect to the primary and issue the command to delete the slot,
but it doesn't exist, not in case of arbitrary errors. I believe there is
precedent for similar behaviour from all the DROP ... IF EXISTS commands.

Re: DROP SUBSCRIPTION with no slot

[Žiga Kranjec]

> On 25 Sep 2019, at 01:22, Jeff Janes  wrote:
> 
> There is no HINT in the error message itself, but there is in the 
> documentation, see note at end of 
> https://www.postgresql.org/docs/current/sql-dropsubscription.html 
> .  I agree 
> with you that the DROP should just work in this case, even more so than in my 
> case.  But if we go with the argument that doing that is too error prone, 
> then do we want to include a HINT on how to be error prone more conveniently?
> 
> Cheers,
> 
> Jeff

Ah. I missed that bit in the documentation!

Perhaps a publication should remember, whether it actually created a 
replication slot and only try to remove it, if it did. Although that probably 
wouldn't help much in your case.


Ž.

Re: Two pg_rewind patches (auto generate recovery conf and ensure clean shutdown)

[Alvaro Herrera]

CC Alexey for reasons that become clear below.

On 2019-Sep-25, Paul Guo wrote:

> > Question about 0003.  If we specify --skip-clean-shutdown and the cluter
> > was not cleanly shut down, shouldn't we error out instead of trying to
> > press on?
> 
> pg_rewind would error out in this case, see sanityChecks().
> Users are expected to clean up themselves if they use this argument.

Ah, good.  We should have a comment about that below the relevant
stanza, I suggest.  (Or maybe in the same comment that ends in line
272).

I pushed 0001 with a few tweaks.  Nothing really substantial, just my
OCD that doesn't leave me alone ... but this means your subsequent
patches need to be adjusted.  One thing is that that patch touched
pg_rewind for no reason (those changes should have been in 0002) --
dropped those.

Another thing in 0002 is that you're adding a "-R" switch to pg_rewind,
but we have another patch in the commitfest using the same switch for a
different purpose.  Maybe you guys need to get to an agreement over who
uses the letter :-)  Also, it would be super helpful if you review
Alexey's patch: https://commitfest.postgresql.org/24/1849/


This line is far too long:

+   printf(_("  -s, --skip-clean-shutdown  skip running single-mode 
postgres if needed to make sure target is clean shutdown\n"));

Can we make the description more concise?

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

Re: PostgreSQL 12 RC1 Press Release Draft

[Jonathan S. Katz]

On 9/25/19 8:21 AM, Erikjan Rijkers wrote:
> On 2019-09-25 13:08, Jonathan S. Katz wrote:
>> On 9/25/19 6:50 AM, Sehrope Sarkuni wrote:
>>> The "Upgrading to PostgreSQL 12 RC 1" references v11 rather than v12:
>>>
>>> "To upgrade to PostgreSQL 11 RC 1 from Beta 4 or an earlier version of
>>> PostgreSQL 11, ..."
> 
> A small typo (or processing hickup):
> 
> 'pattern\_ops'  should be
> 'pattern_ops'

Thanks for noticing that -- because this is Markdown we need to escape
the underscore.

Thanks!

Jonathan



signature.asc
Description: OpenPGP digital signature

Re: PATCH: standby crashed when replay block which truncated in standby but failed to truncate in master node

[Fujii Masao]

On Tue, Sep 24, 2019 at 10:41 AM Michael Paquier  wrote:
>
> On Mon, Sep 23, 2019 at 01:45:14PM +0200, Tomas Vondra wrote:
> > On Mon, Sep 23, 2019 at 03:48:50PM +0800, Thunder wrote:
> >> Is this an issue?
> >> Can we fix like this?
> >> Thanks!
> >>
> >
> > I do think it is a valid issue. No opinion on the fix yet, though.
> > The report was sent on saturday, so patience ;-)
>
> And for some others it was even a longer weekend.  Anyway, the problem
> can be reproduced if you apply the attached which introduces a failure
> point, and then if you run the following commands:
> create table aa as select 1;
> delete from aa;
> \! touch /tmp/truncate_flag
> vacuum aa;
> \! rm /tmp/truncate_flag
> vacuum aa; -- panic on standby
>
> This also points out that there are other things to worry about than
> interruptions, as for example DropRelFileNodeLocalBuffers() could lead
> to an ERROR, and this happens before the physical truncation is done
> but after the WAL record is replayed on the standby, so any failures
> happening at the truncation phase before the work is done would be a
> problem.  However we are talking about failures which should not
> happen and these are elog() calls.  It would be tempting to add a
> critical section here, but we could still have problems if we have a
> failure after the WAL record has been flushed, which means that it
> would be replayed on the standby, and the surrounding comments are
> clear about that.

Could you elaborate what problem adding a critical section there occurs?

Regards,

-- 
Fujii Masao

Re: Proposal for syntax to support creation of partition tables when creating parent table

[Tom Lane]

Muhammad Usama  writes:
> I want to propose an extension to CREATE TABLE syntax to allow the creation
> of partition tables along with its parent table using a single statement.

TBH, I think this isn't a particularly good idea.  It seems very
reminiscent of the variant of CREATE SCHEMA that lets you create
a bunch of contained objects along with the schema.  That variant
is a mess to support and AFAIK it's practically unused in the
real world.  (If it were used, we'd get requests to support more
than the small number of object types that the CREATE SCHEMA
grammar currently allows.)

As Fabien noted, there's been some related discussion about this
area, but nobody was advocating a solution of this particular shape.

regards, tom lane

Re: Proposal for syntax to support creation of partition tables when creating parent table

[Fabien COELHO]

Hello Muhammad,

I think that it may be better to have a partition spec which describes not 
the list of partitions, but what is wanted, letting postgres to do some 
more work.


See this thread:

https://www.postgresql.org/message-id/alpine.DEB.2.21.1907150711080.22273@lancre


I want to propose an extension to CREATE TABLE syntax to allow the creation
of partition tables along with its parent table using a single statement.

In this proposal, I am proposing to specify the list of partitioned tables
after the PARTITION BY clause.

CREATE TABLE table_name (..)
  PARTITION BY { RANGE | LIST | HASH } (..)
(
  list of partitions
)  ;



Below are a few examples of the proposed syntax, in a nutshell, I am
leveraging the syntax currently supported by Postgres for creating
partitioned tables. The purpose of this proposal is to combine the creation
of the parent partition table and its partitions in one SQL statement.

CREATE TABLE Sales (salesman_id INT, salesman_name TEXT, sales_region TEXT,
hiring_date DATE, sales_amount INT )
   PARTITION BY RANGE (hiring_date)
   (
   PARTITION part_one FOR VALUES FROM ('2008-02-01') TO ('2008-03-01'),
   PARTITION part_two FOR VALUES FROM ('2009-02-01') TO ('2009-03-01'),
   PARTITION part_def DEFAULT
   );

CREATE TABLE Sales2 (salesman_id INT, salesman_name TEXT, sales_region
TEXT, hiring_date DATE, sales_amount INT )
   PARTITION BY HASH (salesman_id)
   (
   PARTITION par_one FOR VALUES WITH (MODULUS 2, REMAINDER 0),
   PARTITION par_two FOR VALUES WITH (MODULUS 2, REMAINDER 1)
   );

CREATE TABLE Sales3(salesman_id INT, salesman_name TEXT, sales_region TEXT,
hiring_date DATE, sales_amount INT)
   PARTITION BY LIST (sales_region)
   (
PARTITION pt_one FOR VALUES IN ('JAPAN','CHINA'),
PARTITION pt_two FOR VALUES IN ('USA','CANADA'),
PARTITION pt_def DEFAULT
   );

-- Similarly for specifying subpartitions of partitioned tables

CREATE TABLE All_Sales ( year INT, month INT, day INT, info TEXT)
   PARTITION BY RANGE(year)(
   PARTITION sale_2019_2020 FOR VALUES FROM (2019) TO (2021)
   PARTITION BY LIST(month)
   (
PARTITION sale_2019_2020_1 FOR VALUES IN (1,2,3,4)
PARTITION BY RANGE(day)(
PARTITION sale_2019_2020_1_1 FOR VALUES FROM (1) TO (10)
PARTITION BY HASH(info)
(
 PARTITION sale_2019_2020_1_1_1 FOR VALUES WITH (MODULUS
2,REMAINDER 0),
 PARTITION sale_2019_2020_1_1_2 FOR VALUES WITH (MODULUS
2,REMAINDER 1)
),
PARTITION sale_2019_2020_1_2 FOR VALUES FROM (10) TO (20),
PARTITION sale_2019_2020_1_3 FOR VALUES FROM (20) TO (32)),
PARTITION sale_2019_2020_2 FOR VALUES IN (5,6,7,8),
PARTITION sale_2019_2020_3 FOR VALUES IN (9,10,11,12)
   ),
   PARTITION sale_2021_2022 FOR VALUES FROM (2021) TO (2023),
   PARTITION sale_2023_2024 FOR VALUES FROM (2023) TO (2025),
   PARTITION sale_default default
   );

This new syntax requires minimal changes in the code. I along with my
colleague Movead.li have drafted a rough POC patch attached to this email.

Please note that the patch is just to showcase the new syntax and get a
consensus on the overall design and approach.

As far as I know, there are already few ongoing discussions related to the
partition syntax enhancements, but the proposed syntax will not interfere
with these ongoing proposals. Here is a link to one such discussion:
https://www.postgresql.org/message-id/alpine.DEB.2.21.1907150711080.22273%40lancre

Please feel free to share your thoughts.

Best Regards

...
Muhammad Usama
Highgo Software Canada
URL : http://www.highgo.ca
ADDR: 10318 WHALLEY BLVD, Surrey, BC



--
Fabien Coelho - CRI, MINES ParisTech

Re: [HACKERS] [WIP] Effective storage of duplicates in B-tree index.

[Anastasia Lubennikova]

24.09.2019 3:13, Peter Geoghegan wrote:

On Wed, Sep 18, 2019 at 7:25 PM Peter Geoghegan  wrote:

I attach version 16. This revision merges your recent work on WAL
logging with my recent work on simplifying _bt_dedup_one_page(). See
my e-mail from earlier today for details.

I attach version 17. This version has changes that are focussed on
further polishing certain things, including fixing some minor bugs. It
seemed worth creating a new version for that. (I didn't get very far
with the space utilization stuff I talked about, so no changes there.)

Attached is v18. In this version bt_dedup_one_page() is refactored so that:
- no temp page is used, all updates are applied to the original page.
- each posting tuple wal logged separately.
This also allowed to simplify btree_xlog_dedup significantly.


Another infrastructure thing that the patch needs to handle to be committable:

We still haven't added an "off" switch to deduplication, which seems
necessary. I suppose that this should look like GIN's "fastupdate"
storage parameter. It's not obvious how to do this in a way that's
easy to work with, though. Maybe we could do something like copy GIN's
GinGetUseFastUpdate() macro, but the situation with nbtree is actually
quite different. There are two questions for nbtree when it comes to
deduplication within an inde: 1) Does the user want to use
deduplication, because that will help performance?, and 2) Is it
safe/possible to use deduplication at all?

I'll send another version with dedup option soon.


I think that we should probably stash this information (deduplication
is both possible and safe) in the metapage. Maybe we can copy it over
to our insertion scankey, just like the "heapkeyspace" field -- that
information also comes from the metapage (it's based on the nbtree
version). The "heapkeyspace" field is a bit ugly, so maybe we
shouldn't go further by adding something similar, but I don't see any
great alternative right now.


Why is it necessary to save this information somewhere but rel->rd_options,
while we can easily access this field from _bt_findinsertloc() and 
_bt_load().


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

diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c
index 05e7d67..d65e2a7 100644
--- a/contrib/amcheck/verify_nbtree.c
+++ b/contrib/amcheck/verify_nbtree.c
@@ -145,6 +145,7 @@ static void bt_tuple_present_callback(Relation index, HeapTuple htup,
 	  bool tupleIsAlive, void *checkstate);
 static IndexTuple bt_normalize_tuple(BtreeCheckState *state,
 	 IndexTuple itup);
+static inline IndexTuple bt_posting_logical_tuple(IndexTuple itup, int n);
 static bool bt_rootdescend(BtreeCheckState *state, IndexTuple itup);
 static inline bool offset_is_negative_infinity(BTPageOpaque opaque,
 			   OffsetNumber offset);
@@ -419,12 +420,13 @@ bt_check_every_level(Relation rel, Relation heaprel, bool heapkeyspace,
 		/*
 		 * Size Bloom filter based on estimated number of tuples in index,
 		 * while conservatively assuming that each block must contain at least
-		 * MaxIndexTuplesPerPage / 5 non-pivot tuples.  (Non-leaf pages cannot
-		 * contain non-pivot tuples.  That's okay because they generally make
-		 * up no more than about 1% of all pages in the index.)
+		 * MaxPostingIndexTuplesPerPage / 3 "logical" tuples.  heapallindexed
+		 * verification fingerprints posting list heap TIDs as plain non-pivot
+		 * tuples, complete with index keys.  This allows its heap scan to
+		 * behave as if posting lists do not exist.
 		 */
 		total_pages = RelationGetNumberOfBlocks(rel);
-		total_elems = Max(total_pages * (MaxIndexTuplesPerPage / 5),
+		total_elems = Max(total_pages * (MaxPostingIndexTuplesPerPage / 3),
 		  (int64) state->rel->rd_rel->reltuples);
 		/* Random seed relies on backend srandom() call to avoid repetition */
 		seed = random();
@@ -924,6 +926,7 @@ bt_target_page_check(BtreeCheckState *state)
 		size_t		tupsize;
 		BTScanInsert skey;
 		bool		lowersizelimit;
+		ItemPointer	scantid;
 
 		CHECK_FOR_INTERRUPTS();
 
@@ -994,29 +997,73 @@ bt_target_page_check(BtreeCheckState *state)
 
 		/*
 		 * Readonly callers may optionally verify that non-pivot tuples can
-		 * each be found by an independent search that starts from the root
+		 * each be found by an independent search that starts from the root.
+		 * Note that we deliberately don't do individual searches for each
+		 * "logical" posting list tuple, since the posting list itself is
+		 * validated by other checks.
 		 */
 		if (state->rootdescend && P_ISLEAF(topaque) &&
 			!bt_rootdescend(state, itup))
 		{
 			char	   *itid,
 	   *htid;
+			ItemPointer tid = BTreeTupleGetHeapTID(itup);
 
 			itid = psprintf("(%u,%u)", state->targetblock, offset);
 			htid = psprintf("(%u,%u)",
-			ItemPointerGetBlockNumber(&(itup->t_tid)),
-			ItemPointerGetOffsetNumber(&(itup->t_tid)));
+			

Re: JSONPATH documentation

[Liudmila Mantrova]

On 9/25/19 12:08 AM, Peter Eisentraut wrote:

On 2019-09-23 00:03, Tom Lane wrote:

While we're whining about this, I find it very off-putting that
the jsonpath stuff was inserted in the JSON functions section
ahead of the actual JSON functions.  I think it should have
gone after them, because it feels like a barely-related interjection
as it stands.  Maybe there's even a case that it should be
its own , after the "functions-json" section.

I'd just switch the sect2's around.


As more SQL/JSON functionality gets added, I believe a separate sect1 is 
likely to be more justified. However, for v12 I'd vote for moving sect2 
down. The patch is attached, it also fixes the ambiguous sentence that 
has raised questions in this thread.


--
Liudmila Mantrova
Technical writer at Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 432dbad868..6cb5d6 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -11646,14 +11646,14 @@ table2-mapping
   

 
- SQL/JSON path expressions
- (see ).
+ PostgreSQL-specific functions and operators for JSON
+ data types (see ).
 


 
- PostgreSQL-specific functions and operators for JSON
- data types (see ).
+ SQL/JSON path expressions
+ (see ).
 

   
@@ -11665,1748 +11665,1748 @@ table2-mapping
 see .
   
 
- 
-  SQL/JSON Path Expressions
+  
+  JSON Functions and Operators
   
-SQL/JSON
-path expressions
+JSON
+functions and operators
   
 
   
-   SQL/JSON path expressions specify the items to be retrieved
-   from the JSON data, similar to XPath expressions used
-   for SQL access to XML. In PostgreSQL,
-   path expressions are implemented as the jsonpath
-   data type and can use any elements described in
-   .
+shows the operators that
+   are available for use with JSON data types (see ).
   
 
-  JSON query functions and operators
-   pass the provided path expression to the path engine
-   for evaluation. If the expression matches the queried JSON data,
-   the corresponding SQL/JSON item is returned.
-   Path expressions are written in the SQL/JSON path language
-   and can also include arithmetic expressions and functions.
-   Query functions treat the provided expression as a
-   text string, so it must be enclosed in single quotes.
-  
+  
+ json and jsonb Operators
+ 
+  
+   
+Operator
+Right Operand Type
+Return type
+Description
+Example
+Example Result
+   
+  
+  
+   
+-
+int
+json or jsonb
+Get JSON array element (indexed from zero, negative
+integers count from the end)
+'[{"a":"foo"},{"b":"bar"},{"c":"baz"}]'::json-2
+{"c":"baz"}
+   
+   
+-
+text
+json or jsonb
+Get JSON object field by key
+'{"a": {"b":"foo"}}'::json-'a'
+{"b":"foo"}
+   
+
+-
+int
+text
+Get JSON array element as text
+'[1,2,3]'::json-2
+3
+   
+   
+-
+text
+text
+Get JSON object field as text
+'{"a":1,"b":2}'::json-'b'
+2
+   
+   
+#
+text[]
+json or jsonb
+Get JSON object at the specified path
+'{"a": {"b":{"c": "foo"}}}'::json#'{a,b}'
+{"c": "foo"}
+   
+   
+#
+text[]
+text
+Get JSON object at the specified path as text
+'{"a":[1,2,3],"b":[4,5,6]}'::json#'{a,2}'
+3
+   
+  
+ 
+   
 
+  
+   
+There are parallel variants of these operators for both the
+json and jsonb types.
+The field/element/path extraction operators
+return the same type as their left-hand input (either json
+or jsonb), except for those specified as
+returning text, which coerce the value to text.
+The field/element/path extraction operators return NULL, rather than
+failing, if the JSON input does not have the right structure to match
+the request; for example if no such element exists.  The
+field/element/path extraction operators that accept integer JSON
+array subscripts all support negative subscripting from the end of
+arrays.
+   
+  
   
-   A path expression consists of a sequence of elements allowed
-   by the jsonpath data type.
-   The path expression is evaluated from left to right, but
-   you can use parentheses to change the order of operations.
-   If the evaluation is successful, a sequence of SQL/JSON items
-   (SQL/JSON sequence) is produced,
-   and the evaluation result is returned to the JSON query function
-   that completes the specified computation.
+   The standard comparison operators shown in   are available for
+   jsonb, but not for json. They follow the
+   ordering rules for B-tree operations outlined at .
   
-
 

Proposal for syntax to support creation of partition tables when creating parent table

[Muhammad Usama]

Hi Hackers,

I want to propose an extension to CREATE TABLE syntax to allow the creation
of partition tables along with its parent table using a single statement.

In this proposal, I am proposing to specify the list of partitioned tables
after the PARTITION BY clause.

CREATE TABLE table_name (..)
   PARTITION BY { RANGE | LIST | HASH } (..)
 (
   list of partitions
)  ;

Below are a few examples of the proposed syntax, in a nutshell, I am
leveraging the syntax currently supported by Postgres for creating
partitioned tables. The purpose of this proposal is to combine the creation
of the parent partition table and its partitions in one SQL statement.

CREATE TABLE Sales (salesman_id INT, salesman_name TEXT, sales_region TEXT,
hiring_date DATE, sales_amount INT )
PARTITION BY RANGE (hiring_date)
(
PARTITION part_one FOR VALUES FROM ('2008-02-01') TO ('2008-03-01'),
PARTITION part_two FOR VALUES FROM ('2009-02-01') TO ('2009-03-01'),
PARTITION part_def DEFAULT
);

CREATE TABLE Sales2 (salesman_id INT, salesman_name TEXT, sales_region
TEXT, hiring_date DATE, sales_amount INT )
PARTITION BY HASH (salesman_id)
(
PARTITION par_one FOR VALUES WITH (MODULUS 2, REMAINDER 0),
PARTITION par_two FOR VALUES WITH (MODULUS 2, REMAINDER 1)
);

CREATE TABLE Sales3(salesman_id INT, salesman_name TEXT, sales_region TEXT,
hiring_date DATE, sales_amount INT)
PARTITION BY LIST (sales_region)
(
 PARTITION pt_one FOR VALUES IN ('JAPAN','CHINA'),
 PARTITION pt_two FOR VALUES IN ('USA','CANADA'),
 PARTITION pt_def DEFAULT
);

-- Similarly for specifying subpartitions of partitioned tables

CREATE TABLE All_Sales ( year INT, month INT, day INT, info TEXT)
PARTITION BY RANGE(year)(
PARTITION sale_2019_2020 FOR VALUES FROM (2019) TO (2021)
PARTITION BY LIST(month)
(
 PARTITION sale_2019_2020_1 FOR VALUES IN (1,2,3,4)
 PARTITION BY RANGE(day)(
 PARTITION sale_2019_2020_1_1 FOR VALUES FROM (1) TO (10)
 PARTITION BY HASH(info)
 (
  PARTITION sale_2019_2020_1_1_1 FOR VALUES WITH (MODULUS
2,REMAINDER 0),
  PARTITION sale_2019_2020_1_1_2 FOR VALUES WITH (MODULUS
2,REMAINDER 1)
 ),
 PARTITION sale_2019_2020_1_2 FOR VALUES FROM (10) TO (20),
 PARTITION sale_2019_2020_1_3 FOR VALUES FROM (20) TO (32)),
 PARTITION sale_2019_2020_2 FOR VALUES IN (5,6,7,8),
 PARTITION sale_2019_2020_3 FOR VALUES IN (9,10,11,12)
),
PARTITION sale_2021_2022 FOR VALUES FROM (2021) TO (2023),
PARTITION sale_2023_2024 FOR VALUES FROM (2023) TO (2025),
PARTITION sale_default default
);

This new syntax requires minimal changes in the code. I along with my
colleague Movead.li have drafted a rough POC patch attached to this email.

Please note that the patch is just to showcase the new syntax and get a
consensus on the overall design and approach.

As far as I know, there are already few ongoing discussions related to the
partition syntax enhancements, but the proposed syntax will not interfere
with these ongoing proposals. Here is a link to one such discussion:
https://www.postgresql.org/message-id/alpine.DEB.2.21.1907150711080.22273%40lancre

Please feel free to share your thoughts.

Best Regards

...
Muhammad Usama
Highgo Software Canada
URL : http://www.highgo.ca
ADDR: 10318 WHALLEY BLVD, Surrey, BC


new_partition_syntax_poc.diff
Description: Binary data

Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions

[Amit Kapila]

On Tue, Sep 3, 2019 at 4:30 AM Alvaro Herrera  wrote:
>
> In the interest of moving things forward, how far are we from making
> 0001 committable?  If I understand correctly, the rest of this patchset
> depends on https://commitfest.postgresql.org/24/944/ which seems to be
> moving at a glacial pace (or, actually, slower, because glaciers do
> move, which cannot be said of that other patch.)
>

I am not sure if it is completely correct that the other part of the
patch is dependent on that CF entry.  I have studied both the threads
(not every detail) and it seems to me it is dependent on one of the
patches from that series which handles concurrent aborts.  It is patch
0003-Gracefully-handle-concurrent-aborts-of-uncommitted-t.Jan4.patch
from what the Nikhil has posted on that thread [1].  Am, I wrong?

So IIUC, the problem of concurrent aborts is that if we allow catalog
scans for in-progress transactions, then we might get wrong answers in
cases where somebody has performed Alter-Abort-Alter which is clearly
explained with an example in email [2].  To solve that problem Nikhil
seems to have written a patch [1] which detects these concurrent
aborts during a system table scan and then aborts the decoding of such
a transaction.

Now, the problem is that patch has written considering 2PC
transactions and might not deal with all cases for in-progress
transactions especially when sub-transactions are involved as alluded
by Arseny Sher [3].  So, the problem seems to be for cases when some
sub-transaction aborts, but the main transaction still continued and
we try to decode it.  Nikhil's patch won't be able to deal with it
because I think it just checks top-level xid whereas for this we need
to check all-subxids which I think is possible now as Tomas seems to
have written WAL for each xid-assignment.  It might or might not be
the best solution to check the status of all-subxids, but I think
first we need to agree that the problem is just for concurrent aborts
and that we can solve it by using some part of the technology being
developed as part of patch "Logical decoding of two-phase
transactions" (https://commitfest.postgresql.org/24/944/) rather than
the entire patchset.

I hope I am not saying something very obvious here and it helps in
moving this patch forward.

Thoughts?

[1] - 
https://www.postgresql.org/message-id/CAMGcDxcBmN6jNeQkgWddfhX8HbSjQpW%3DUo70iBY3P_EPdp%2BLTQ%40mail.gmail.com
[2] - 
https://www.postgresql.org/message-id/EEBD82AA-61EE-46F4-845E-05B94168E8F2%40postgrespro.ru
[3] - https://www.postgresql.org/message-id/87a7py4iwl.fsf%40ars-thinkpad

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

Re: PostgreSQL 12 RC1 Press Release Draft

[Erikjan Rijkers]

On 2019-09-25 13:08, Jonathan S. Katz wrote:

On 9/25/19 6:50 AM, Sehrope Sarkuni wrote:

The "Upgrading to PostgreSQL 12 RC 1" references v11 rather than v12:

"To upgrade to PostgreSQL 11 RC 1 from Beta 4 or an earlier version of
PostgreSQL 11, ..."


A small typo (or processing hickup):

'pattern\_ops'  should be
'pattern_ops'





Thanks! Fixed attached,

Jonathan

Re: Cache lookup errors with functions manipulation object addresses

[Alvaro Herrera]

On 2019-Sep-25, Michael Paquier wrote:

> On Tue, Sep 24, 2019 at 03:25:08PM +0900, Kyotaro Horiguchi wrote:
> > In 0003, empty string and NULL are not digtinguishable in psql
> > text output. It'd be better that the regression test checks that
> > the return is actually NULL using "is NULL" or "\pset null
> > ''" or something like.
> 
> For a patch whose purpose is to check after NULL, I missed to consider
> that...  Thanks!  I have just added a "\pset null NULL" because that's
> less bulky than the other option, and all the results properly show
> NULL, without any empty strings around.  I am not sending a new patch
> set just for that change though, and the most recent version is here:
> https://github.com/michaelpq/postgres/tree/objaddr_nulls

I have still to review this comprehensively, but one thing I don't quite
like is the shape of the new regression tests, which seem a bit too
bulky ... why not use the style elsewhere in that file, with a large
VALUES clause to supply input params for a single query?  That would be
a lot faster, too.

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

Re: Zedstore - compressed in-core columnar storage

[Ashutosh Sharma]

Hi Alexandra,

On Tue, Sep 17, 2019 at 4:45 PM Ashutosh Sharma  wrote:
>
> On Thu, Aug 29, 2019 at 5:39 PM Heikki Linnakangas  wrote:
> >
> > On 29/08/2019 14:30, Ashutosh Sharma wrote:
> > >
> > > On Wed, Aug 28, 2019 at 5:30 AM Alexandra Wang  > > > wrote:
>
> Further, the UPDATE operation on zedstore table is very slow. I think
> that's because in case of zedstore table we have to update all the
> btree data structures even if one column is updated and that really
> sucks. Please let me know if there is some other reason for it.
>

There was no answer for this in your previous reply. It seems like you
missed it. As I said earlier, I tried performing UPDATE operation with
optimised build and found that to update around 10 lacs record in
zedstore table it takes around 24k ms whereas for normal heap table it
takes 2k ms. Is that because in case of zedstore table we have to
update all the Btree data structures even if one column is updated or
there is some other reason for it. If yes, could you please let us
know. FYI, I'm trying to update the table with just two columns.

Further, In the latest code I'm getting this warning message when it
is compiled using -O2 optimisation flag.

zedstore_tidpage.c: In function ‘zsbt_collect_dead_tids’:
zedstore_tidpage.c:978:10: warning: ‘page’ may be used uninitialized
in this function [-Wmaybe-uninitialized]
   opaque = ZSBtreePageGetOpaque(page);
  ^
Attached is the patch that fixes it.

-- 
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com
diff --git a/src/backend/access/zedstore/zedstore_tidpage.c b/src/backend/access/zedstore/zedstore_tidpage.c
index 7730ef3..f590f79 100644
--- a/src/backend/access/zedstore/zedstore_tidpage.c
+++ b/src/backend/access/zedstore/zedstore_tidpage.c
@@ -956,9 +956,10 @@ zsbt_collect_dead_tids(Relation rel, zstid starttid, zstid *endtid, uint64 *num_
 			buf = zsbt_descend(rel, ZS_META_ATTRIBUTE_NUM, nexttid, 0, true);
 			if (!BufferIsValid(buf))
 return result;
-			page = BufferGetPage(buf);
 		}
 
+		page = BufferGetPage(buf);
+
 		maxoff = PageGetMaxOffsetNumber(page);
 		for (off = FirstOffsetNumber; off <= maxoff; off++)
 		{

Re: PostgreSQL 12 RC1 Press Release Draft

[Jonathan S. Katz]

On 9/25/19 6:50 AM, Sehrope Sarkuni wrote:
> The "Upgrading to PostgreSQL 12 RC 1" references v11 rather than v12:
> 
> "To upgrade to PostgreSQL 11 RC 1 from Beta 4 or an earlier version of
> PostgreSQL 11, ..."

Thanks! Fixed attached,

Jonathan
PostgreSQL 12 RC 1 Released
===

The PostgreSQL Global Development Group announces that the first release
candidate of PostgreSQL 12 is now available for download. As a release
candidate, PostgreSQL 12 RC 1 should be identical to the initial release of
PostgreSQL 12, though some more fixes may be applied prior to the general
availability of PostgreSQL 12.

The planned date for the general availability of PostgreSQL 12 is October 3,
2019. Please see the "Release Schedule" section for more details.

Upgrading to PostgreSQL 12 RC 1
---

To upgrade to PostgreSQL 12 RC 1 from Beta 4 or an earlier version of
PostgreSQL 12, you will need to use a strategy similar to upgrading between
major versions of PostgreSQL (e.g. `pg_upgrade` or `pg_dump` / `pg_restore`).
For more information, please visit the documentation section on
[upgrading](https://www.postgresql.org/docs/12/static/upgrading.html).

Changes Since 12 Beta 4
---

There have been many bug fixes for PostgreSQL 12 reported during the Beta 4
period and applied to this release candidate. These include:

* Add additional "leakproof" markings to certain string functions to better support nondeterministic collations. This can positively impact the performance of some query plans
* Removal of the ECPG `DECLARE STATEMENT` functionality
* The ecpglib major version change was reverted
* Fix handling of nondeterministic collations with pattern\_ops opclasses

For a detailed list of fixes, please visit the
[open items](https://wiki.postgresql.org/wiki/PostgreSQL_12_Open_Items#resolved_before_12rc1)
page.

Release Schedule


This is the first release candidate for PostgreSQL 12. Unless an issue is
discovered that warrants a delay or to produce an additional release candidate,
PostgreSQL 12 should be made generally available on October 3, 2019.

For further information please see the
[Beta Testing](https://www.postgresql.org/developer/beta/) page.

Links
-

* [Download](https://www.postgresql.org/download/)
* [Beta Testing Information](https://www.postgresql.org/developer/beta/)
* [PostgreSQL 12 RC Release Notes](https://www.postgresql.org/docs/12/static/release-12.html)
* [PostgreSQL 12 Open Issues](https://wiki.postgresql.org/wiki/PostgreSQL_12_Open_Items)
* [Submit a Bug](https://www.postgresql.org/account/submitbug/)


signature.asc
Description: OpenPGP digital signature

Re: row filtering for logical replication

[Euler Taveira]

Em seg, 23 de set de 2019 às 01:59, movead li  escreveu:
>
> I find several problems as below when I test the patches:
>
First of all, thanks for your review.

> 1. There be some regression problem after apply 0001.patch~0005.patch
>The regression problem is solved in 0006.patch
>
Which regression?

> 2. There be a data wrong after create subscription if the relation contains
>  inherits table, for example:
>
Ouch. Good catch! Forgot about the ONLY in COPY with query. I will add
a test for it.

> 3. I am puzzled when I test the update.
>   Use the tables in problem 2 and test as below:
>   #
>   On publication:
>   postgres=# insert into cities values('t1',123, 34);
>   INSERT 0 1
>
INSERT isn't replicated.

>   postgres=# update cities SET altitude = 134 where altitude = 34;
>   UPDATE 1
>
There should be an error because you don't have a PK or REPLICA IDENTITY.

postgres=# update cities SET altitude = 134 where altitude = 34;
ERROR:  cannot update table "cities" because it does not have a
replica identity and publishes updates
HINT:  To enable updating the table, set REPLICA IDENTITY using ALTER TABLE.

Even if you create a PK or REPLICA IDENTITY, it won't turn this UPDATE
into a INSERT and send it to the other node (indeed UPDATE will be
sent however there isn't a tuple to update). Also, filter columns must
be in PK or REPLICA IDENTITY. I explain this in documentation.

> 4. SQL splicing code in fetch_remote_table_info() function is too long
>
I split it into small pieces. I also run pgindent to improve code style.

I'll send a patchset later today.


Regards,


--
   Euler Taveira   Timbira -
http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

Re: PostgreSQL 12 RC1 Press Release Draft

[Sehrope Sarkuni]

The "Upgrading to PostgreSQL 12 RC 1" references v11 rather than v12:

"To upgrade to PostgreSQL 11 RC 1 from Beta 4 or an earlier version of
PostgreSQL 11, ..."

Regards,
-- Sehrope Sarkuni
Founder & CEO | JackDB, Inc. | https://www.jackdb.com/

PostgreSQL 12 RC1 Press Release Draft

[Jonathan S. Katz]

Hi,

Attached is a draft for the PostgreSQL 12 RC1 press release. Please let
me know if you find any errors or notable omissions.

I'd also like to take this opportunity as a chance to say thank you to
everyone for your hard work to get PostgreSQL 12 to this point. I'm
personally very excited for what should be yet another fantastic release
that will provide a lot of great features and enhancements for our users!

Thanks,

Jonathan
PostgreSQL 12 RC 1 Released
===

The PostgreSQL Global Development Group announces that the first release
candidate of PostgreSQL 12 is now available for download. As a release
candidate, PostgreSQL 12 RC 1 should be identical to the initial release of
PostgreSQL 12, though some more fixes may be applied prior to the general
availability of PostgreSQL 12.

The planned date for the general availability of PostgreSQL 12 is October 3,
2019. Please see the "Release Schedule" section for more details.

Upgrading to PostgreSQL 12 RC 1
---

To upgrade to PostgreSQL 11 RC 1 from Beta 4 or an earlier version of
PostgreSQL 11, you will need to use a strategy similar to upgrading between
major versions of PostgreSQL (e.g. `pg_upgrade` or `pg_dump` / `pg_restore`).
For more information, please visit the documentation section on
[upgrading](https://www.postgresql.org/docs/12/static/upgrading.html).

Changes Since 12 Beta 4
---

There have been many bug fixes for PostgreSQL 12 reported during the Beta 4
period and applied to this release candidate. These include:

* Add additional "leakproof" markings to certain string functions to better support nondeterministic collations. This can positively impact the performance of some query plans
* Removal of the ECPG `DECLARE STATEMENT` functionality
* The ecpglib major version change was reverted
* Fix handling of nondeterministic collations with pattern\_ops opclasses

For a detailed list of fixes, please visit the
[open items](https://wiki.postgresql.org/wiki/PostgreSQL_12_Open_Items#resolved_before_12rc1)
page.

Release Schedule


This is the first release candidate for PostgreSQL 12. Unless an issue is
discovered that warrants a delay or to produce an additional release candidate,
PostgreSQL 12 should be made generally available on October 3, 2019.

For further information please see the
[Beta Testing](https://www.postgresql.org/developer/beta/) page.

Links
-

* [Download](https://www.postgresql.org/download/)
* [Beta Testing Information](https://www.postgresql.org/developer/beta/)
* [PostgreSQL 12 RC Release Notes](https://www.postgresql.org/docs/12/static/release-12.html)
* [PostgreSQL 12 Open Issues](https://wiki.postgresql.org/wiki/PostgreSQL_12_Open_Items)
* [Submit a Bug](https://www.postgresql.org/account/submitbug/)


signature.asc
Description: OpenPGP digital signature

Re: Batch insert in CTAS/MatView code

[Asim R P]

On Mon, Sep 9, 2019 at 4:02 PM Paul Guo  wrote:
>
> So in theory
> we should not worry about additional tuple copy overhead now, and then I
tried the patch without setting
> multi-insert threshold as attached.
>

I reviewed your patch today.  It looks good overall.  My concern is that
the ExecFetchSlotHeapTuple call does not seem appropriate.  In a generic
place such as createas.c, we should be using generic tableam API only.
However, I can also see that there is no better alternative.  We need to
compute the size of accumulated tuples so far, in order to decide whether
to stop accumulating tuples.  There is no convenient way to obtain the
length of the tuple, given a slot.  How about making that decision solely
based on number of tuples, so that we can avoid ExecFetchSlotHeapTuple call
altogether?

The multi insert copy code deals with index tuples also, which I don't see
in the patch.  Don't we need to consider populating indexes?

Asim

Re: Index Skip Scan

[Dmitry Dolgov]

> On Wed, Sep 25, 2019 at 3:03 AM Kyotaro Horiguchi  
> wrote:
>
> At Tue, 24 Sep 2019 09:06:27 -0300, Alvaro Herrera  
> wrote in <20190924120627.GA12454@alvherre.pgsql>
> > On 2019-Sep-24, Kyotaro Horiguchi wrote:
> >
> > > Sorry, it's not a boolean. A tristate value. From the definition
> > > (Back, NoMove, Forward) = (-1, 0, 1), (dir1 == -dir2) if
> > > NoMovement did not exist. If it is not guranteed,
> > >
> > > (dir1 != 0 && dir1 == -dir2) ?
> >
> > Maybe just add ScanDirectionIsOpposite(dir1, dir2) with that
> > definition? :-)
>
> Yeah, sounds good to establish it as a part of ScanDirection's
> definition.

Yep, this way looks better.


v27-0001-Index-skip-scan.patch
Description: Binary data

Re: [PATCH] Sort policies and triggers by table name in pg_dump.

[Benjie Gillam]

> Thanks.  Perhaps you could add your patch to the next commit fest
> then at https://commitfest.postgresql.org/25/?

Thanks, submitted.

Re: [PATCH] Sort policies and triggers by table name in pg_dump.

[Michael Paquier]

On Tue, Sep 24, 2019 at 08:48:33AM +0100, Benjie Gillam wrote:
> Here we create two identically named triggers and two identically
> named policies on tables foo and bar. If instead we ran these
> statements in a different order (or if the object IDs were to wrap)
> the order of the pg_dump would be different even though the
> databases are identical other than object IDs. The attached
> patch eliminates this difference.

Thanks.  Perhaps you could add your patch to the next commit fest
then at https://commitfest.postgresql.org/25/?

This way, your patch gains more visibility for potential reviews.
Another key thing to remember is that one patch authored requires one
other patch of equal difficulty to be reviewed.
--
Michael


signature.asc
Description: PGP signature

Re: PATCH: standby crashed when replay block which truncated in standby but failed to truncate in master node

[Kyotaro Horiguchi]

At Wed, 25 Sep 2019 12:24:03 +0900, Michael Paquier  wrote 
in <20190925032403.gf1...@paquier.xyz>
> On Tue, Sep 24, 2019 at 02:48:16PM +0900, Kyotaro Horiguchi wrote:
> > Of course I found no *explicit* ones. But I found one
> > ereport(DEBUG1 in register_dirty_segment. So it will work at
> > least for the case where fsync request queue is full.
> 
> Exactly.  I have not checked the patch in details, but I think that
> we should not rely on the assumption that no code paths in this area do
> not check after CHECK_FOR_INTERRUPTS() as smgrtruncate() does much
> more than just the physical segment truncation.

Agreed to the point. Just I doubted that it really fixes the
author's problem. And confirmed that it can be.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: PostgreSQL12 and older versions of OpenSSL

[Michael Paquier]

On Tue, Sep 24, 2019 at 11:25:30AM -0400, Tom Lane wrote:
> Alvaro Herrera  writes:
>> ... I wonder if we should really continue to support
>> OpenSSL 0.9.8.
> 
> Fair question, but post-rc1 is no time to be moving that goalpost
> for the v12 branch.

Yeah.  I worked in the past with SUSE-based appliances, and I recall
that those folks have been maintaining their own patched version of
OpenSSL 0.9.8 with a bunch of custom patches, some of them coming from
newer versions of upstream to take care of security issues with 0.9.8.
So even if they call their version 0.9.8j, I think that they include
much more security-related fixes than their version string suggests.
I don't know at which extent though.

>> Anyway I suppose it's not impossible that third parties are still
>> maintaining their 1.0.0 branch,
> 
> Another data point on that is that Red Hat is still supporting
> 1.0.1e in RHEL6.  I don't think we should assume that just because
> OpenSSL upstream has dropped support for a branch, it no longer
> exists in the wild.
> 
> Having said that, if it makes our lives noticeably easier to
> drop support for 0.9.8 in HEAD, I won't stand in the way.

Agreed.  There is an argument for dropping support for OpenSSL 0.9.8
in 13~, but I don't agree of doing that in 12.  Let's just fix the
issue.
--
Michael


signature.asc
Description: PGP signature

Re: Remove page-read callback from XLogReaderState.

[Kyotaro Horiguchi]

Hello.

709d003fbd hit this. Rebased.

Works fine but needs detailed verification and maybe further
cosmetic fixes.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From 6be22b10c9df068c53c98ad3106e1bd88e07aeeb Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Thu, 5 Sep 2019 20:21:55 +0900
Subject: [PATCH v7 1/4] Move callback-call from ReadPageInternal to
 XLogReadRecord.

The current WAL record reader reads page data using a call back
function.  Although it is not so problematic alone, it would be a
problem if we are going to do add tasks like encryption which is
performed on page data before WAL reader reads them. To avoid that the
record reader facility has to have a new code path corresponds to
every new callback, this patch separates page reader from WAL record
reading facility by modifying the current WAL record reader to a state
machine.

As the first step of that change, this patch moves the page reader
function out of ReadPageInternal, then the remaining tasks of the
function are taken over by the new function XLogNeedData. As the
result XLogPageRead directly calls the page reader callback function
according to the feedback from XLogNeedData.
---
 src/backend/access/transam/xlog.c  |  16 +-
 src/backend/access/transam/xlogreader.c| 332 +++--
 src/backend/access/transam/xlogutils.c |  10 +-
 src/backend/replication/logical/logicalfuncs.c |   2 +-
 src/backend/replication/walsender.c|  10 +-
 src/bin/pg_rewind/parsexlog.c  |  16 +-
 src/bin/pg_waldump/pg_waldump.c|   8 +-
 src/include/access/xlogreader.h|  23 +-
 src/include/access/xlogutils.h |   2 +-
 src/include/replication/logicalfuncs.h |   2 +-
 10 files changed, 259 insertions(+), 162 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 6c69eb6dd7..5dcb2e500c 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -884,7 +884,7 @@ static bool InstallXLogFileSegment(XLogSegNo *segno, char *tmppath,
 static int	XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
 		 int source, bool notfoundOk);
 static int	XLogFileReadAnyTLI(XLogSegNo segno, int emode, int source);
-static int	XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
+static bool	XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
 		 int reqLen, XLogRecPtr targetRecPtr, char *readBuf);
 static bool WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
 		bool fetching_ckpt, XLogRecPtr tliRecPtr);
@@ -4249,7 +4249,6 @@ ReadRecord(XLogReaderState *xlogreader, XLogRecPtr RecPtr, int emode,
 	XLogRecord *record;
 	XLogPageReadPrivate *private = (XLogPageReadPrivate *) xlogreader->private_data;
 
-	/* Pass through parameters to XLogPageRead */
 	private->fetching_ckpt = fetching_ckpt;
 	private->emode = emode;
 	private->randAccess = (RecPtr != InvalidXLogRecPtr);
@@ -11522,7 +11521,7 @@ CancelBackup(void)
  * XLogPageRead() to try fetching the record from another source, or to
  * sleep and retry.
  */
-static int
+static bool
 XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen,
 			 XLogRecPtr targetRecPtr, char *readBuf)
 {
@@ -11581,7 +11580,8 @@ retry:
 			readLen = 0;
 			readSource = 0;
 
-			return -1;
+			xlogreader->readLen = -1;
+			return false;
 		}
 	}
 
@@ -11676,7 +11676,8 @@ retry:
 		goto next_record_is_invalid;
 	}
 
-	return readLen;
+	xlogreader->readLen = readLen;
+	return true;
 
 next_record_is_invalid:
 	lastSourceFailed = true;
@@ -11690,8 +11691,9 @@ next_record_is_invalid:
 	/* In standby-mode, keep trying */
 	if (StandbyMode)
 		goto retry;
-	else
-		return -1;
+
+	xlogreader->readLen = -1;
+	return false;
 }
 
 /*
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 27c27303d6..c2bb664f07 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -34,9 +34,9 @@
 static void report_invalid_record(XLogReaderState *state, const char *fmt,...)
 			pg_attribute_printf(2, 3);
 static bool allocate_recordbuf(XLogReaderState *state, uint32 reclength);
-static int	ReadPageInternal(XLogReaderState *state, XLogRecPtr pageptr,
-			 int reqLen);
-static void XLogReaderInvalReadState(XLogReaderState *state);
+static bool XLogNeedData(XLogReaderState *state, XLogRecPtr pageptr,
+		 int reqLen, bool header_inclusive);
+static void XLogReaderDiscardReadingPage(XLogReaderState *state);
 static bool ValidXLogRecordHeader(XLogReaderState *state, XLogRecPtr RecPtr,
   XLogRecPtr PrevRecPtr, XLogRecord *record, bool randAccess);
 static bool ValidXLogRecord(XLogReaderState *state, XLogRecord *record,
@@ -104,7 +104,7 @@ XLogReaderAllocate(int wal_segment_size, const char *waldir,
 	/* system_identifier initialized to zeroes above */