Re: [HACKERS] delta relations in AFTER triggers

[Craig Ringer]

On 22 Nov. 2016 03:01, "Thomas Munro"  wrote:


 How about a QueryEnvironment (as shown in the
patch I posted) that contains a list of NamedTuplestore pointers (the
SpiRelation struct in the patch I posted, renamed) and in future
perhaps lists of other ephemeral/transient objects that we want to
expose to SQL?


Very good idea. Sooner or later someone will probably want query-scoped
variables like MS-SQL and MySQL for example.

Re: [HACKERS] delta relations in AFTER triggers

[Craig Ringer]

On 22 Nov. 2016 01:05, "Kevin Grittner"  wrote:



Right, I think we should assume that there will be other ways
people want to use parts of what is done here, including building
tuplestores through other means and referencing them in queries.


Yes. PL/pgsql table-valued variables for one thing.

Re: [HACKERS] Trouble building uuid-ossp extension in new versions of Postgres

[Tom Lane]

Ryan Murphy  writes:
> I have been trying to build Postgres and migrate my data to the newest
> version.  Postgres builds just fine, but I also need the uuid-ossp module,
> which used to build fine for me and now does not...

I think the key question is what version of the ossp code are you using?
We've had lots of reports of *that* failing to build on modern platforms,
but since development on it stopped years ago, it's not exactly a moving
target.  I'm surprised to hear of a case where you can still build the
ossp code but our code then fails.  Also, when was "used to"?

> The code that is failing to build dates to Tom Lane's commit
> b8cc8f94730610c0189aa82dfec4ae6ce9b13e34 in which he is apparently creating
> an abstraction layer for uuid-ossp to be built with any of 3 different
> backends.  I was looking for documentation about how to choose a backend /
> more details on how to build this extension now, but drawing a blank.
> Again I am on a Mac, this is my compiler info:

On Mac, the recommended thing is to forget about the ossp code and
use "configure --with-uuid=e2fs".  Sorry if that wasn't clear enough.

Reading over your post again, it sounds like you're trying to force-build
contrib/uuid-ossp without having used any of the configure options that
are supposed to enable it.  I'm not sure that would ever have worked very
reliably.

regards, tom lane


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

Re: [HACKERS] delta relations in AFTER triggers

[Kevin Grittner]

On Sun, Dec 4, 2016 at 11:35 PM, Haribabu Kommi
 wrote:

> Moved to next CF with "waiting on author" status.

Patch v8 attempts to address the issues explicitly raised in
Thomas Munro's review.  An opaque query environment is created
that, for now, only passes through ephemeral named relations, of
which the only initial implementation is named tuplestores; but
the techniques are intended to support both other types of
ephemeral named relations and environmental properties (things
affecting parsing, planning, and execution that are not coming from
the system catalog) besides ENRs.  There is no clue in the access
to the QE whether something is, for example, stored in a list or a
hash table.  That's on purpose, so that the addition of other
properties or changes to their implementation doesn't affect the
calling code.

There were a few changes Thomas included in the version he posted,
without really delving into an explanation for those changes.  Some
or all of them are likely to be worthwhile, but I would rather
incorporate them based on explicit discussion, so this version
doesn't do much other than generalize the interface a little,
change some names, and add more regression tests for the new
feature.  (The examples I worked up for the rough proof of concept
of enforcement of RI through set logic rather than row-at-a-time
navigation were the basis for the new tests, so the idea won't get
totally lost.)  Thomas, please discuss each suggested change (e.g.,
the inclusion of the query environment in the parameter list of a
few more functions).

Changed to "Needs review" status.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


transition-v8.diff.gz
Description: GNU Zip compressed data

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

[HACKERS] Trouble building uuid-ossp extension in new versions of Postgres

[Ryan Murphy]

Hello,

I have been trying to build Postgres and migrate my data to the newest
version.  Postgres builds just fine, but I also need the uuid-ossp module,
which used to build fine for me and now does not...

I am currently "git pull"ed to commit b645a05fc6112a4857ceac574d4aa2
4174a70417.

I cd into /contrib/uuid-ossp, and type "make" and this happens
(on Mac, see compiler version below):

$ make
gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute
-Wformat-security -fno-strict-aliasing -fwrapv
-Wno-unused-command-line-argument
-O2  -I../../contrib/pgcrypto -I. -I. -I../../src/include   -c -o
uuid-ossp.o uuid-ossp.c

uuid-ossp.c:282:23: error: use of undeclared identifier 'uuid_s_ok'
uint32_tstatus = uuid_s_ok;
 ^
uuid-ossp.c:285:5: warning: implicit declaration of function 'uuid_create'
is invalid in C99 [-Wimplicit-function-declaration]
uuid_create(, );
^
uuid-ossp.c:287:19: error: use of undeclared identifier 'uuid_s_ok'
if (status == uuid_s_ok)
  ^
uuid-ossp.c:289:6: warning: implicit declaration of function
'uuid_to_string' is invalid in C99 [-Wimplicit-function-declaration]
uuid_to_string(, , );
^
uuid-ossp.c:290:20: error: use of undeclared identifier 'uuid_s_ok'
if (status == uuid_s_ok)
  ^
uuid-ossp.c:306:19: error: use of undeclared identifier 'uuid_s_ok'
if (status != uuid_s_ok)

...


I can post the rest of the errors if needed but it just keeps going,
eventually erroring out with "Too many errors".  Indeed I do not see the
identifier 'uuid_s_ok' defined anywhere within the contrib/uuid-ossp tree.

The code that is failing to build dates to Tom Lane's commit
b8cc8f94730610c0189aa82dfec4ae6ce9b13e34 in which he is apparently creating
an abstraction layer for uuid-ossp to be built with any of 3 different
backends.  I was looking for documentation about how to choose a backend /
more details on how to build this extension now, but drawing a blank.

Again I am on a Mac, this is my compiler info:

$ clang -v
Apple LLVM version 7.0.2 (clang-700.1.79)
Target: x86_64-apple-darwin14.0.0
Thread model: posix

Thanks much!
Ryan

Re: [HACKERS] Speedup twophase transactions

[Bruce Momjian]

On Sat, Dec 17, 2016 at 07:38:46PM -0500, Bruce Momjian wrote:
> As Andres already stated, the problem is that there is a text/plain and
> text/html of the same email, and the diff is _inside_ the multipa/mixed
> HTML block.  I think it needs to be outside on its own.

Mutt shows text/plain by default.  I bet email readers that prefer to
display HTML see the patch.  Mystery solved by Andres!  :-)

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


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

Re: [HACKERS] Speedup twophase transactions

[Bruce Momjian]

On Sat, Dec 17, 2016 at 03:47:34PM -0800, Andres Freund wrote:
> On 2016-12-17 15:30:08 -0800, David Fetter wrote:
> > On Sat, Dec 17, 2016 at 05:54:04PM -0500, Bruce Momjian wrote:
> > > On Sun, Dec 18, 2016 at 07:41:50AM +0900, Michael Paquier wrote:
> > > > On Sun, Dec 18, 2016 at 6:42 AM, Bruce Momjian  wrote:
> > > > > Uh, did you mean to attached patch here?
> > > >
> > > > Strange. I can confirm that I have received the patch as attached, but
> > > > it is not on the archives.
> > >
> > > It must have been stripped by our email system.  You were a direct
> > > CC so you received it.
> >
> > I was neither, and I received it, so I don't thing PostgreSQL's email
> > system stripped it.  It's pretty mystifying, though.
> 
> The mime construction in the email is weird. The attachement is below a
> multipart/alternative and multipart/mixed, besides the text/html version
> of the plain text email.  Because the attachement is below the
> multipart/alternative (i.e. the switch between plain text / html), it'll
> not be considered a proper attachement by many mail readers.
> 
> It seems quite borked to put an attachement there - weird that apple
> mail does so.

Oh, I now see the attachment _is_ in the original email, it is just that
mutt doesn't show it.  Here is the heirarchy of the email as shown by
mutt:

  I 1   
  [multipa/alternativ, 7bit, 27K]
  I 2 ├─>   
 [text/plain, quoted, us-ascii, 0.8K]
  I 3 └─>   
   [multipa/mixed, 7bit, 26K]
  I 4   ├─> 
  [text/html, quoted, us-ascii, 5.4K]
  A 5   ├─>twophase_recovery_list.diff  
  [applica/octet-stre, 7bit, 20K]
  I 6   └─> 
[text/html, 7bit, us-ascii, 0.4K]

As Andres already stated, the problem is that there is a text/plain and
text/html of the same email, and the diff is _inside_ the multipa/mixed
HTML block.  I think it needs to be outside on its own.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


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

Re: [HACKERS] Speedup twophase transactions

[Andres Freund]

On 2016-12-17 15:30:08 -0800, David Fetter wrote:
> On Sat, Dec 17, 2016 at 05:54:04PM -0500, Bruce Momjian wrote:
> > On Sun, Dec 18, 2016 at 07:41:50AM +0900, Michael Paquier wrote:
> > > On Sun, Dec 18, 2016 at 6:42 AM, Bruce Momjian  wrote:
> > > > Uh, did you mean to attached patch here?
> > >
> > > Strange. I can confirm that I have received the patch as attached, but
> > > it is not on the archives.
> >
> > It must have been stripped by our email system.  You were a direct
> > CC so you received it.
>
> I was neither, and I received it, so I don't thing PostgreSQL's email
> system stripped it.  It's pretty mystifying, though.

The mime construction in the email is weird. The attachement is below a
multipart/alternative and multipart/mixed, besides the text/html version
of the plain text email.  Because the attachement is below the
multipart/alternative (i.e. the switch between plain text / html), it'll
not be considered a proper attachement by many mail readers.

It seems quite borked to put an attachement there - weird that apple
mail does so.

Andres


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

Re: [HACKERS] Speedup twophase transactions

[David Fetter]

On Sat, Dec 17, 2016 at 05:54:04PM -0500, Bruce Momjian wrote:
> On Sun, Dec 18, 2016 at 07:41:50AM +0900, Michael Paquier wrote:
> > On Sun, Dec 18, 2016 at 6:42 AM, Bruce Momjian  wrote:
> > > Uh, did you mean to attached patch here?
> > 
> > Strange. I can confirm that I have received the patch as attached, but
> > it is not on the archives.
> 
> It must have been stripped by our email system.  You were a direct
> CC so you received it.

I was neither, and I received it, so I don't thing PostgreSQL's email
system stripped it.  It's pretty mystifying, though.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


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

Re: [HACKERS] Speedup twophase transactions

[Stas Kelvich]

> On 18 Dec 2016, at 01:54, Bruce Momjian  wrote:
> 
> On Sun, Dec 18, 2016 at 07:41:50AM +0900, Michael Paquier wrote:
>> 
>> 
>> Strange. I can confirm that I have received the patch as attached, but
>> it is not on the archives.
> 
> It must have been stripped by our email system.  You were a direct CC so
> you received it.
> 

Then, probably, my mail client did something strange. I’ll check.

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


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

Re: [HACKERS] Speedup twophase transactions

[Bruce Momjian]

On Sun, Dec 18, 2016 at 07:41:50AM +0900, Michael Paquier wrote:
> On Sun, Dec 18, 2016 at 6:42 AM, Bruce Momjian  wrote:
> > Uh, did you mean to attached patch here?
> 
> Strange. I can confirm that I have received the patch as attached, but
> it is not on the archives.

It must have been stripped by our email system.  You were a direct CC so
you received it.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


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

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Michael Paquier]

On Sun, Dec 18, 2016 at 3:59 AM, Robert Haas  wrote:
> On Fri, Dec 16, 2016 at 5:30 PM, Michael Paquier
>  wrote:
>> From the discussions of last year on -hackers, it was decided to *not*
>> have an additional column per complains from a couple of hackers
>> (Robert you were in this set at this point), ...
>
> Hmm, I don't recall taking that position, but then there are a lot of
> things that I ought to recall and don't.  (Ask my wife!)

[... digging objects of the past ...]
>From the past thread:
https://www.postgresql.org/message-id/ca+tgmoy790rphhbogxmbtg6mzsendoxdbxebekaet9zpz8g...@mail.gmail.com
The complain is directed directly to multiple verifiers per users
though, not to have the type in a separate column.
-- 
Michael


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

Re: [HACKERS] Speedup twophase transactions

[Michael Paquier]

On Sun, Dec 18, 2016 at 6:42 AM, Bruce Momjian  wrote:
> Uh, did you mean to attached patch here?

Strange. I can confirm that I have received the patch as attached, but
it is not on the archives.
-- 
Michael
diff --git a/src/backend/access/transam/twophase.c 
b/src/backend/access/transam/twophase.c
index 5415604..fb69646 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -45,8 +45,8 @@
  *   fsynced
  * * If COMMIT happens after checkpoint then backend reads state 
data from
  *   files
- * * In case of crash replay will move data from xlog to files, if 
that
- *   hasn't happened before. XXX TODO - move to shmem in replay 
also
+ * * Simplified version of the same scenario happens during 
recovery and
+ *   replication. See comments to KnownPreparedXact structure.
  *
  *-
  */
@@ -181,6 +181,35 @@ static GlobalTransaction MyLockedGxact = NULL;
 
 static bool twophaseExitRegistered = false;
 
+/*
+ * During replay and replication KnownPreparedList holds info about active 
prepared
+ * transactions that weren't moved to files yet. We will need that info by the 
end of
+ * recovery (including promote) to restore memory state of that transactions.
+ *
+ * Naive approach here is to move each PREPARE record to disk, fsync it and 
don't have
+ * that list at all, but that provokes a lot of unnecessary fsyncs on small 
files
+ * causing replica to be slower than master.
+ *
+ * Replay of twophase records happens by the following rules:
+ * * On PREPARE redo KnownPreparedAdd() is called to add that 
transaction to
+ *   KnownPreparedList and no more actions are taken.
+ * * On checkpoint redo we iterate through KnownPreparedList and 
move all prepare
+ *   records that behind redo_horizon to files and deleting them 
from list.
+ * * On COMMIT/ABORT we delete file or entry in KnownPreparedList.
+ * * At the end of recovery we move all known prepared 
transactions to disk
+ *   to allow 
RecoverPreparedTransactions/StandbyRecoverPreparedTransactions
+ *   do their work.
+ */
+typedef struct KnownPreparedXact
+{
+   TransactionId   xid;
+   XLogRecPtr  prepare_start_lsn;
+   XLogRecPtr  prepare_end_lsn;
+   dlist_node  list_node;
+} KnownPreparedXact;
+
+static dlist_head KnownPreparedList = DLIST_STATIC_INIT(KnownPreparedList);
+
 static void RecordTransactionCommitPrepared(TransactionId xid,
int nchildren,
TransactionId 
*children,
@@ -1241,9 +1270,9 @@ ReadTwoPhaseFile(TransactionId xid, bool give_warnings)
  * Reads 2PC data from xlog. During checkpoint this data will be moved to
  * twophase files and ReadTwoPhaseFile should be used instead.
  *
- * Note clearly that this function accesses WAL during normal operation, 
similarly
- * to the way WALSender or Logical Decoding would do. It does not run during
- * crash recovery or standby processing.
+ * Note clearly that this function can access WAL during normal operation, 
similarly
+ * to the way WALSender or Logical Decoding would do.
+ *
  */
 static void
 XlogReadTwoPhaseData(XLogRecPtr lsn, char **buf, int *len)
@@ -1252,8 +1281,6 @@ XlogReadTwoPhaseData(XLogRecPtr lsn, char **buf, int *len)
XLogReaderState *xlogreader;
char   *errormsg;
 
-   Assert(!RecoveryInProgress());
-
xlogreader = XLogReaderAllocate(_local_xlog_page, NULL);
if (!xlogreader)
ereport(ERROR,
@@ -1691,6 +1718,15 @@ PrescanPreparedTransactions(TransactionId **xids_p, int 
*nxids_p)
int nxids = 0;
int allocsize = 0;
 
+   /*
+* Move prepared transactions from KnownPreparedList to files, if any.
+* It is possible to skip that step and teach subsequent code about
+* KnownPreparedList, but whole PrescanPreparedTransactions() happens
+* once during end of recovery or promote, so probably it isn't worth
+* complications.
+*/
+   KnownPreparedRecreateFiles(InvalidXLogRecPtr);
+
cldir = AllocateDir(TWOPHASE_DIR);
while ((clde = ReadDir(cldir, TWOPHASE_DIR)) != NULL)
{
@@ -2162,3 +2198,111 @@ RecordTransactionAbortPrepared(TransactionId xid,
 */
SyncRepWaitForLSN(recptr, false);
 }
+
+/*
+ * KnownPreparedAdd.
+ *
+ * Store correspondence of start/end lsn and xid in KnownPreparedList.
+ * This is called during redo of prepare record to have list of prepared
+ * transactions that aren't yet moved to 2PC files by the end of recovery.
+ */
+void
+KnownPreparedAdd(XLogReaderState *record)
+{
+   

Re: [HACKERS] Proposal for changes to recovery.conf API

[Bruce Momjian]

On Sat, Dec 17, 2016 at 02:52:41PM +0100, Magnus Hagander wrote:
> The point is that the documentation about the recovery.conf changes in
> Postgres are only interesting to people migrating to Postgres 10, i.e.
> this is not quality documentation for someone going from Postgres 10 to
> Postgres 11.
> 
> 
> 
> They will also be interesting to people going from 9.4 to 11, or from 9.3 to
> 12. Or from 9.5 to 13.
> 
> They only become uninteresting when we stop supporting 9.6 which is the last
> version that didn't have that functionality. 

No, they become uninteresting to anyone who has passed Postgres 10.  I
would argue they are still required to be around even after we stop
supporting Postgres 9.6 because we know everyone will not upgrade off of
supported releases once we stop supporting them.  This means we can
effectively never remove the information.

Right now if you migrate from Postgres 8.0 to Postgres 9.6, all the
information you need is in the 9.6 documentation.  If you were to remove
migration details from 8.4 to 9.0 they would have to look at the 9.0
docs to get a full picture of how to migrate.

Again, I am fine putting this as a subsection of the release notes, but
let's not pretend it is some extra section we can remove in five years.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


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

Re: [HACKERS] Speedup twophase transactions

[Bruce Momjian]

On Fri, Dec 16, 2016 at 02:00:46PM +0300, Stas Kelvich wrote:
> 
> > On 27 Sep 2016, at 03:30, Michael Paquier  wrote:
> > 
> > OK. I am marking this patch as returned with feedback then. Looking
> > forward to seeing the next investigations.. At least this review has
> > taught us one thing or two.
> 
> So, here is brand new implementation of the same thing.
> 
> Now instead of creating pgproc entry for prepared transaction during recovery,
> I just store recptr/xid correspondence in separate 2L-list and deleting 
> entries in that
> list if redo process faced commit/abort. In case of checkpoint or end of 
> recovery
> transactions remaining in that list are dumped to files in pg_twophase.
> 
> Seems that current approach is way more simpler and patch has two times less
> LOCs then previous one.

Uh, did you mean to attached patch here?

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


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

Re: [HACKERS] CREATE OR REPLACE VIEW bug

[Tom Lane]

Dean Rasheed  writes:
> Attached is a patch enforcing this order and adding some comments to
> make it clear why the order matters here.
> Barring objections I'll back-patch this to 9.4 where WCO was added.

Looks reasonable in a quick once-over.  I didn't test it.

regards, tom lane


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

Re: [HACKERS] Rethinking our fulltext phrase-search implementation

[Tom Lane]

I wrote:
> It's worth noting that with these rules, phrase searches will act as
> though "!x" always matches somewhere; for instance "!a <-> !b" will match
> any tsvector.  I argue that this is not wrong, not even if the tsvector is
> empty: there could have been adjacent stopwords matching !a and !b in the
> original text.  Since we've adjusted the phrase matching rules to treat
> stopwords as unknown-but-present words in a phrase, I think this is
> consistent.  It's also pretty hard to assert this is wrong and at the same
> time accept "!a <-> b" matching b at the start of the document.

To clarify this point, I'm imagining that the patch would include
documentation changes like the attached.

regards, tom lane

diff --git a/doc/src/sgml/datatype.sgml b/doc/src/sgml/datatype.sgml
index 67d0c34..464ce83 100644
*** a/doc/src/sgml/datatype.sgml
--- b/doc/src/sgml/datatype.sgml
*** SELECT 'fat  rat  ! cat'::tsqu
*** 3959,3973 
  tsquery 
  
   'fat'  'rat'  !'cat'
- 
- SELECT '(fat | rat) - cat'::tsquery;
-   tsquery
- ---
-  'fat' - 'cat' | 'rat' - 'cat'
  
- 
-  The last example demonstrates that tsquery sometimes
-  rearranges nested operators into a logically equivalent formulation.
  
  
  
--- 3959,3965 
diff --git a/doc/src/sgml/textsearch.sgml b/doc/src/sgml/textsearch.sgml
index 2da7595..bc33a70 100644
*** a/doc/src/sgml/textsearch.sgml
--- b/doc/src/sgml/textsearch.sgml
*** text @@ text
*** 323,328 
--- 323,330 
  at least one of its arguments must appear, while the ! (NOT)
  operator specifies that its argument must not appear in
  order to have a match.
+ For example, the query fat  ! rat matches documents that
+ contain fat but not rat.
 
  
 
*** SELECT phraseto_tsquery('the cats ate th
*** 377,382 
--- 379,401 
  then , then -,
  and ! most tightly.
 
+ 
+
+ It's worth noticing that the AND/OR/NOT operators mean something subtly
+ different when they are within the arguments of a FOLLOWED BY operator
+ than when they are not, because then the position of the match is
+ significant.  Normally, !x matches only documents that do not
+ contain x anywhere.  But x - !y
+ matches x if it is not immediately followed by y;
+ an occurrence of y elsewhere in the document does not prevent
+ a match.  Another example is that x  y normally only
+ requires that x and y both appear somewhere in the
+ document, but (x  y) - z requires x
+ and y to match at the same place, immediately before
+ a z.  Thus this query behaves differently from x
+ - z  y - z, which would match a document
+ containing two separate sequences x z and y z.
+

  


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

Re: [HACKERS] PSQL commands: \quit_if, \quit_unless

[Corey Huinker]

>
> > (Or in other words, let's see \while ... \endloop in the minimal proposal
> > as well, or at least a sketch of how to get there.)
>
> It seems to me that we could implement \if ... \else ...\endif by
> having some kind of stack that indicates which constructs we're inside
> of and whether we're currently executing commands or discarding them.
> I think we want to avoid waiting until we see \endif to start running
> commands; it's better to execute or skip/discard each command as we
> see it.
>

+1


>
> To implement \while, we'd also need to remember previous commands so
> that when we reach the end of the loop, we can rewind and put all of
> those commands back on the stack to be executed again, or perhaps to
> be skipped if the \while condition turns out now to be false.
>

This might be what you meant when you said "those commands back on the
stack", but I think we'd have to remember not a list of commands, but the
raw string of bytes from the start of the \while to the \endwhile (or
equivalent), because any psql vars within that block could themselves be a
non-parameter part of a command:

  -- this is how I fake an 'exit 0' now:
  \set work_needs_to_be_done t
  select
case
  when :'work_needs_to_be_done'::boolean then ''
  else '\q'
end as cmd
  \gset
  :cmd

  -- ridiculous example to illustrate complications in remembering past
commands
  select
case
   when random() < 0.5 then '\ir my_script.sql'
   when random() < 0.7 'select * from a_table; select count(*) from
another_table;'
   else 'select null as foo;'
end as cmd
  \gset
  :cmd

And even then, things get complicated, because an \ir include which makes
it this iteration might not make it the next, and the \endwhile might have
been inside that include, or vice-versa, an included file starts a \while
it doesn't finish.

So maybe what we store is a stack of buffers that are currently open (STDIN
being captured as a buffer only when a \while starts, everything else being
files), and additionally have a stack of positions where a \while started
(buffer_id, position in buffer).

Additionally, we could assert that all \while-\endwhile pairs must happen
in the same MainLoop (aka file), and mismatches are an error.

I'm happy to keep sketching out what control structures might look like and
how to implement them. But I'm also happy to leave while/for loops out for
now.

Re: [HACKERS] CREATE OR REPLACE VIEW bug

[Dean Rasheed]

On 17 December 2016 at 15:42, Dean Rasheed  wrote:
> It seems that there is a bug in CREATE OR REPLACE VIEW...
>
> DefineView()/DefineVirtualRelation() will need a little re-jigging to
> do things in the required order.

...and the required order for existing views is

1. Add any new columns
2. Add rules to store the new query
3. Update the view options

because 2 will fail if the view's columns don't match the query's columns.

Attached is a patch enforcing this order and adding some comments to
make it clear why the order matters here.

Barring objections I'll back-patch this to 9.4 where WCO was added.

Regards,
Dean
diff --git a/src/backend/commands/view.c b/src/backend/commands/view.c
new file mode 100644
index c6b0e4f..414507f
--- a/src/backend/commands/view.c
+++ b/src/backend/commands/view.c
@@ -59,15 +59,13 @@ validateWithCheckOption(char *value)
 /*-
  * DefineVirtualRelation
  *
- * Create the "view" relation. `DefineRelation' does all the work,
- * we just provide the correct arguments ... at least when we're
- * creating a view.  If we're updating an existing view, we have to
- * work harder.
+ * Create a view relation and use the rules system to store the query
+ * for the view.
  *-
  */
 static ObjectAddress
 DefineVirtualRelation(RangeVar *relation, List *tlist, bool replace,
-	  List *options)
+	  List *options, Query *viewParse)
 {
 	Oid			viewOid;
 	LOCKMODE	lockmode;
@@ -162,18 +160,13 @@ DefineVirtualRelation(RangeVar *relation
 		checkViewTupleDesc(descriptor, rel->rd_att);
 
 		/*
-		 * The new options list replaces the existing options list, even if
-		 * it's empty.
-		 */
-		atcmd = makeNode(AlterTableCmd);
-		atcmd->subtype = AT_ReplaceRelOptions;
-		atcmd->def = (Node *) options;
-		atcmds = lappend(atcmds, atcmd);
-
-		/*
 		 * If new attributes have been added, we must add pg_attribute entries
 		 * for them.  It is convenient (although overkill) to use the ALTER
 		 * TABLE ADD COLUMN infrastructure for this.
+		 *
+		 * Note that we must do this before updating the query for the view,
+		 * since the rules system requires that the correct view columns be in
+		 * place when defining the new rules.
 		 */
 		if (list_length(attrList) > rel->rd_att->natts)
 		{
@@ -192,9 +185,38 @@ DefineVirtualRelation(RangeVar *relation
 atcmd->def = (Node *) lfirst(c);
 atcmds = lappend(atcmds, atcmd);
 			}
+
+			AlterTableInternal(viewOid, atcmds, true);
+
+			/* Make the new view columns visible */
+			CommandCounterIncrement();
 		}
 
-		/* OK, let's do it. */
+		/*
+		 * Update the query for the view.
+		 *
+		 * Note that we must do this before updating the view options, because
+		 * the new options may not be compatible with the old view query (for
+		 * example if we attempt to add the WITH CHECK OPTION, we require that
+		 * the new view be automatically updatable, but the old view may not
+		 * have been).
+		 */
+		StoreViewQuery(viewOid, viewParse, replace);
+
+		/* Make the new view query visible */
+		CommandCounterIncrement();
+
+		/*
+		 * Finally update the view options.
+		 *
+		 * The new options list replaces the existing options list, even if
+		 * it's empty.
+		 */
+		atcmd = makeNode(AlterTableCmd);
+		atcmd->subtype = AT_ReplaceRelOptions;
+		atcmd->def = (Node *) options;
+		atcmds = list_make1(atcmd);
+
 		AlterTableInternal(viewOid, atcmds, true);
 
 		ObjectAddressSet(address, RelationRelationId, viewOid);
@@ -211,7 +233,7 @@ DefineVirtualRelation(RangeVar *relation
 		ObjectAddress address;
 
 		/*
-		 * now set the parameters for keys/inheritance etc. All of these are
+		 * Set the parameters for keys/inheritance etc. All of these are
 		 * uninteresting for views...
 		 */
 		createStmt->relation = relation;
@@ -224,13 +246,20 @@ DefineVirtualRelation(RangeVar *relation
 		createStmt->if_not_exists = false;
 
 		/*
-		 * finally create the relation (this will error out if there's an
-		 * existing view, so we don't need more code to complain if "replace"
-		 * is false).
+		 * Create the relation (this will error out if there's an existing
+		 * view, so we don't need more code to complain if "replace" is
+		 * false).
 		 */
 		address = DefineRelation(createStmt, RELKIND_VIEW, InvalidOid, NULL,
  NULL);
 		Assert(address.objectId != InvalidOid);
+
+		/* Make the new view relation visible */
+		CommandCounterIncrement();
+
+		/* Store the query for the view */
+		StoreViewQuery(address.objectId, viewParse, replace);
+
 		return address;
 	}
 }
@@ -530,16 +559,7 @@ DefineView(ViewStmt *stmt, const char *q
 	 * aborted.
 	 */
 	address = DefineVirtualRelation(view, viewParse->targetList,
-	stmt->replace, stmt->options);
-
-	/*
-	 * The relation we have just created is not visible to any other commands
-	 * running with the same transaction & command id. So, increment 

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Robert Haas]

On Fri, Dec 16, 2016 at 5:30 PM, Michael Paquier
 wrote:
> On Sat, Dec 17, 2016 at 5:42 AM, Stephen Frost  wrote:
>> * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
>>> On 12/15/16 8:40 AM, Stephen Frost wrote:
>>> > I don't follow why we can't change the syntax for CREATE USER to allow
>>> > specifying the verifier type independently.
>>>
>>> That's what the last patch set I looked at actually does.
>>
>> Well, same here, but it was quite a while ago and things have progressed
>> since then wrt SCRAM, as I understand it...
>
> From the discussions of last year on -hackers, it was decided to *not*
> have an additional column per complains from a couple of hackers
> (Robert you were in this set at this point), ...

Hmm, I don't recall taking that position, but then there are a lot of
things that I ought to recall and don't.  (Ask my wife!)

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


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

Re: [HACKERS] Proposal for changes to recovery.conf API

[Robert Haas]

On Sat, Dec 17, 2016 at 8:52 AM, Magnus Hagander  wrote:
> On Sat, Dec 17, 2016 at 5:42 AM, Bruce Momjian  wrote:
>> On Fri, Dec 16, 2016 at 08:52:25PM -0500, Robert Haas wrote:
>> > On Fri, Dec 16, 2016 at 8:07 PM, Tom Lane  wrote:
>> > > I'm still not seeing any value in putting this sort of info into
>> > > a documentation section that's distinct from the release notes.
>> > > We've used links to wiki pages in the past when the information
>> > > seemed to be in flux, and that's reasonable.  But what's the point
>> > > of just linking to somewhere else in the same document?
>> >
>> > If the explanation is just a few sentences long, I see no reason not
>> > to include it in the release notes.  But if it's comparable in length
>> > to a moderately-long chapter then why would we not make it its own
>> > chapter?  I think your argument boils down to "people probably don't
>> > need very much detail about this".  But I think that's the wrong line
>> > of thinking.  In my view, we ought to ship just about as much quality
>> > documentation as people are willing to write.  Saying that we're going
>> > to reject good-quality documentation because we don't want to have too
>> > much of it is like saying we want to reject good-quality features
>> > because we don't want to have too many of them, or good-quality
>> > regression tests because we don't want to have too much code coverage,
>>
>> [ I stopped reading after this. ]
>>
>> The point is that the documentation about the recovery.conf changes in
>> Postgres are only interesting to people migrating to Postgres 10, i.e.
>> this is not quality documentation for someone going from Postgres 10 to
>> Postgres 11.
>>
>
> They will also be interesting to people going from 9.4 to 11, or from 9.3 to
> 12. Or from 9.5 to 13.
>
> They only become uninteresting when we stop supporting 9.6 which is the last
> version that didn't have that functionality.

Right, exactly.  Also, even if it were true that they were only
interesting to people migrating to version 10, that doesn't mean we
shouldn't have them.

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


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

Re: [HACKERS] PSQL commands: \quit_if, \quit_unless

[Robert Haas]

On Fri, Dec 16, 2016 at 12:40 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> So I think it would be reasonable for somebody to implement \if,
>> \elseif, \endif first, with the argument having to be, precisely, a
>> single variable and nothing else (not even a negator).  Then a future
>> patch could allow an expression there instead of a variable.  I don't
>> think that would be any harder to review than going all the way to #5
>> in one shot, and actually it might be simpler.
>
> This seems like a reasonable implementation plan to me, not least because
> it tackles the hard part first.  There's no doubt that we can build an
> expression evaluator, but I'm not entirely sure how we're going to wedge
> conditional eval or loops into psql's command reader.
>
> (Or in other words, let's see \while ... \endloop in the minimal proposal
> as well, or at least a sketch of how to get there.)

It seems to me that we could implement \if ... \else ...\endif by
having some kind of stack that indicates which constructs we're inside
of and whether we're currently executing commands or discarding them.
I think we want to avoid waiting until we see \endif to start running
commands; it's better to execute or skip/discard each command as we
see it.

To implement \while, we'd also need to remember previous commands so
that when we reach the end of the loop, we can rewind and put all of
those commands back on the stack to be executed again, or perhaps to
be skipped if the \while condition turns out now to be false.

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


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

[HACKERS] Rethinking our fulltext phrase-search implementation

[Tom Lane]

I've been thinking about how to fix the problem Andreas Seltenreich
reported at
https://postgr.es/m/87eg1y2s3x@credativ.de

The core of that problem is that the phrase-search patch attempts to
restructure tsquery trees so that there are no operators underneath a
PHRASE operator, except possibly other PHRASE operators.  The goal is
to not have to deal with identifying specific match locations except
while processing a PHRASE operator.  Well, that's an OK idea if it can
be implemented reasonably, but there are several problems with the code
as it stands:

1. The transformation is done by normalize_phrase_tree(), which is
currently invoked (via cleanup_fakeval_and_phrase()) in a rather
ad-hoc set of places including tsqueryin() and the various variants
of to_tsquery().  This leaves lots of scope for errors of omission,
which is exactly the proximate cause of Andreas' bug: ts_rewrite() is
neglecting to re-normalize its result tsquery.  I have little faith
that there aren't other similar errors of omission today, and even
less that we won't introduce more in future.

2. Because the transformation is invoked as early as tsqueryin(),
it's user-visible:

regression=# select 'a <-> (b | c)'::tsquery;
  tsquery  
---
 'a' <-> 'b' | 'a' <-> 'c'
(1 row)

This is confusing to users, and I do not think it's a good idea to
expose what's basically an optimization detail this way.  For stored
tsqueries, we're frozen into this approach forever even if we later
decide that checking for 'a' twice isn't such a hot idea.

3. Worse, early application of the transformation creates problems for
operations such as ts_rewrite: a rewrite might fail to match, or produce
surprising results, because the query trees actually being operated on
aren't what the user probably thinks they are.  At a minimum, to get
consistent results I think we'd have to re-normalize after *each step*
of ts_rewrite, not only at the end.  That would be expensive, and it's
far from clear that it would eliminate all the surprises.

4. The transformations are wrong anyway.  The OR case I showed above is
all right, but as I argued in <24331.1480199...@sss.pgh.pa.us>, the AND
case is not:

regression=# select 'a <-> (b & c)'::tsquery;
  tsquery  
---
 'a' <-> 'b' & 'a' <-> 'c'
(1 row)

This matches 'a b a c', because 'a <-> b' and 'a <-> c' can each be
matched at different places in that text; but it seems highly unlikely to
me that that's what the writer of such a query wanted.  (If she did want
that, she would write it that way to start with.)  NOT is not very nice
either:

regression=# select '!a <-> !b'::tsquery;
  tsquery   

 !'a' & !( !( 'a' <-> 'b' ) & 'b' )
(1 row)

If you dig through that, you realize that the <-> test is pointless;
the query can't match any vector containing 'a', so certainly the <->
condition can't succeed, making this an expensive way to spell "!a & !b".
And that's not the right semantics anyway: if 'x y' matches this query,
which it does and should, why doesn't 'x y a' match?

5. The case with only one NOT under <-> looks like this:

regression=# select '!a <-> b'::tsquery;
tsquery 

 !( 'a' <-> 'b' ) & 'b'
(1 row)

This is more or less in line with the naive view of what its semantics
should be, although I notice that it will match a 'b' at position 1,
which might be a surprising result.  We're not out of the woods though:
this will (and should) match, eg, 'c b a'.  But that means that '!a & b'
is not a safe lossy approximation to '!a <-> b', which is an assumption
that is wired into a number of places.  Simple testing shows that indeed
GIN and GIST index searches get the wrong answers, different from what
you get in a non-indexed search, for queries like this.


So we have a mess here, which needs to be cleaned up quite aside from the
fact that it's capable of provoking Assert failures and/or crashes.

I thought for awhile about moving the normalize_phrase_tree() work
to occur at the start of tsquery execution, rather than in tsqueryin()
et al.  That addresses points 1..3, but doesn't by itself do anything
for points 4 or 5.  Also, it would be expensive because right now
execution works directly from the flat tsquery representation; there's no
conversion to an explicit tree structure on which normalize_phrase_tree()
could conveniently be applied.  Nor do we know at the start whether the
tsquery contains any PHRASE operators.

On the whole, it seems like the best fix would be to get rid of
normalize_phrase_tree() altogether, and instead fix the TS_execute()
engine so it can cope with regular operators underneath phrase operators.
We can still have the optimization of not worrying about lexeme locations
when no phrase operator has been seen, but we have to change
TS_phrase_execute to cope with plain AND/OR/NOT operators and calculate
proper location 

Re: [HACKERS] Logical Replication WIP

[Steve Singer]

On 12/16/2016 07:49 AM, Petr Jelinek wrote:

Hi,

attached is version 13 of the patch.

I merged in changes from PeterE. And did following changes:
- fixed the ownership error messages for both provider and subscriber
- added ability to send invalidation message to invalidate whole
relcache and use it in publication code
- added the post creation/alter/drop hooks
- removed parts of docs that refer to initial sync (which does not exist
yet)
- added timeout handling/retry, etc to apply/launcher based on the GUCs
that exist for wal receiver (they could use renaming though)
- improved feedback behavior
- apply worker now uses owner of the subscription as connection user
- more tests
- check for max_replication_slots in launcher
- clarify the update 'K' sub-message description in protocol


A few things I've noticed so far

If I shutdown the publisher I see the following in the log

2016-12-17 11:33:49.548 EST [1891] LOG:  worker process: ?)G? (PID 1987) 
exited with exit code 1


but then if I shutdown the subscriber postmaster and restart it switches to
2016-12-17 11:43:09.628 EST [2373] LOG:  worker process:  (PID 2393) 
exited with exit code 1


Not sure where the 'G' was coming from (other times I have seen an 'I' 
here or other random characters)



I don't think we are cleaning up subscriptions on a drop database

If I do the following

1) Create a subscription in a new database
2) Stop the publisher
3) Drop the database on the subscriber

test=# create subscription mysuba connection 'host=localhost dbname=test 
port=5440' publication mypub;

test=# \c b
b=# drop database test;
DROP DATABASE
b=# select * FROM pg_subscription ;
 subdbid | subname | subowner | subenabled | subconninfo  | 
subslotname | subpublications

-+-+--++--+-+-
   16384 | mysuba  |   10 | t  | host=localhost dbname=test 
port=5440 | mysuba  | {mypub}


b=# select datname FROM pg_database where oid=16384;
 datname
-
(0 rows)

Also I don't think I can now drop mysuba
b=# drop subscription mysuba;
ERROR:  subscription "mysuba" does not exist











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

Re: [HACKERS] jsonb_delete with arrays

[Dmitry Dolgov]

> Attached is an implantation of jsonb_delete that instead of taking a
single key to remove accepts an array of keys

Since I already saw this patch, here is my small review.

Speaking about implementation of `jsonb_delete_array` - it's fine, but I
would like to suggest two modifications:

* create a separate helper function for jsonb delete operation, to use it
in both `jsonb_delete` and `jsonb_delete_array`. It will help to
concentrate related logic in one place.

* use variadic arguments for `jsonb_delete_array`. For rare cases, when
someone decides to use this function directly instead of corresponding
operator. It will be more consistent with `jsonb_delete` from my point of
view, because it's transition from `jsonb_delete(data, 'key')` to
`jsonb_delete(data, 'key1', 'key2')` is more smooth, than to
`jsonb_delete(data, '{key1, key2}')`.

I've attached a patch with these modifications. What do you think?
diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index 17ee4e4..aa8156e 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -149,6 +149,11 @@ static void setPathArray(JsonbIterator **it, Datum *path_elems,
 			 int level, Jsonb *newval, uint32 nelems, int op_type);
 static void addJsonbToParseState(JsonbParseState **jbps, Jsonb *jb);
 
+/* common workers for jsonb_delete functions */
+static Datum jsonb_delete_worker(Jsonb *in, Datum *elems,
+ bool *nulls, int len);
+
+
 /* state for json_object_keys */
 typedef struct OkeysState
 {
@@ -3395,14 +3400,8 @@ jsonb_delete(PG_FUNCTION_ARGS)
 {
 	Jsonb	   *in = PG_GETARG_JSONB(0);
 	text	   *key = PG_GETARG_TEXT_PP(1);
-	char	   *keyptr = VARDATA_ANY(key);
-	int			keylen = VARSIZE_ANY_EXHDR(key);
-	JsonbParseState *state = NULL;
-	JsonbIterator *it;
-	JsonbValue	v,
-			   *res = NULL;
-	bool		skipNested = false;
-	JsonbIteratorToken r;
+	Datum	   *keys = (Datum *) palloc(sizeof(Datum));
+	bool	   *nulls = (bool *) palloc(sizeof(bool));
 
 	if (JB_ROOT_IS_SCALAR(in))
 		ereport(ERROR,
@@ -3412,21 +3411,95 @@ jsonb_delete(PG_FUNCTION_ARGS)
 	if (JB_ROOT_COUNT(in) == 0)
 		PG_RETURN_JSONB(in);
 
+	keys[0] = PointerGetDatum(key);
+	nulls[0] = FALSE;
+
+	return jsonb_delete_worker(in, keys, nulls, 1);
+}
+
+/*
+ * SQL function jsonb_delete (jsonb, text[])
+ *
+ * return a copy of the jsonb with the indicated items
+ * removed.
+ */
+Datum
+jsonb_delete_array(PG_FUNCTION_ARGS)
+{
+	Jsonb	   *in = PG_GETARG_JSONB(0);
+	ArrayType  *keys = PG_GETARG_ARRAYTYPE_P(1);
+	Datum	   *keys_elems;
+	bool	   *keys_nulls;
+	int			keys_len;
+
+	if (ARR_NDIM(keys) > 1)
+		ereport(ERROR,
+(errcode(ERRCODE_ARRAY_SUBSCRIPT_ERROR),
+ errmsg("wrong number of array subscripts")));
+
+	if (JB_ROOT_IS_SCALAR(in))
+		ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("cannot delete from scalar")));
+
+	if (JB_ROOT_COUNT(in) == 0)
+		PG_RETURN_JSONB(in);
+
+	deconstruct_array(keys, TEXTOID, -1, false, 'i',
+	  _elems, _nulls, _len);
+
+	return jsonb_delete_worker(in, keys_elems, keys_nulls, keys_len);
+}
+
+Datum
+jsonb_delete_worker(Jsonb *in, Datum *elems, bool *nulls, int len)
+{
+	JsonbParseState *state = NULL;
+	JsonbIterator *it;
+	JsonbValue	v,
+			   *res = NULL;
+	bool		skipNested = false;
+	JsonbIteratorToken r;
+
+	if (len == 0)
+		PG_RETURN_JSONB(in);
+
 	it = JsonbIteratorInit(>root);
 
 	while ((r = JsonbIteratorNext(, , skipNested)) != 0)
 	{
 		skipNested = true;
 
-		if ((r == WJB_ELEM || r == WJB_KEY) &&
-			(v.type == jbvString && keylen == v.val.string.len &&
-			 memcmp(keyptr, v.val.string.val, keylen) == 0))
+		if ((r == WJB_ELEM || r == WJB_KEY) && v.type == jbvString)
 		{
-			/* skip corresponding value as well */
-			if (r == WJB_KEY)
-JsonbIteratorNext(, , true);
+			int			i;
+			bool		found = false;
 
-			continue;
+			for (i = 0; i < len; i++)
+			{
+char	   *keyptr;
+int			keylen;
+
+if (nulls[i])
+	continue;
+
+keyptr = VARDATA_ANY(elems[i]);
+keylen = VARSIZE_ANY_EXHDR(elems[i]);
+if (keylen == v.val.string.len &&
+	memcmp(keyptr, v.val.string.val, keylen) == 0)
+{
+	found = true;
+	break;
+}
+			}
+			if (found)
+			{
+/* skip corresponding value as well */
+if (r == WJB_KEY)
+	JsonbIteratorNext(, , true);
+
+continue;
+			}
 		}
 
 		res = pushJsonbValue(, r, r < WJB_BEGIN_ARRAY ?  : NULL);
diff --git a/src/include/catalog/pg_operator.h b/src/include/catalog/pg_operator.h
index 26fa618..347b2e1 100644
--- a/src/include/catalog/pg_operator.h
+++ b/src/include/catalog/pg_operator.h
@@ -1820,6 +1820,8 @@ DATA(insert OID = 3284 (  "||"	   PGNSP PGUID b f f 3802 3802 3802 0 0 jsonb_con
 DESCR("concatenate");
 DATA(insert OID = 3285 (  "-"	   PGNSP PGUID b f f 3802 25 3802 0 0 3302 - - ));
 DESCR("delete object field");
+DATA(insert OID = 3344 (  "-"  PGNSP PGUID b f f 3802 1009 3802 0 0 3343 - -));
+DESCR("delete object fields");
 DATA(insert OID = 3286 (  "-"	   PGNSP PGUID 

[HACKERS] CREATE OR REPLACE VIEW bug

[Dean Rasheed]

It seems that there is a bug in CREATE OR REPLACE VIEW's handling of
WITH CHECK OPTION (noticed while thinking about the recent change to
pg_dump's handling of circular dependencies in views -- d8c05af). If
you use CREATE OR REPLACE VIEW on a view that isn't auto-updatable and
turn it into one that is, and at the same time attempt to add a WITH
CHECK OPTION (which is exactly what pg_dump will now do) it fails:

CREATE TABLE t1 (a int);
CREATE VIEW v1 AS SELECT null::int AS a;
CREATE OR REPLACE VIEW v1 AS SELECT * FROM t1 WHERE a > 0 WITH CHECK OPTION;

ERROR:  WITH CHECK OPTION is supported only on automatically updatable views
HINT:  Views that do not select from a single table or view are not
automatically updatable.


The problem is that before updating the view's query, DefineView()
calls DefineVirtualRelation() which attempts to add the new check
option to the existing view via the ALTER VIEW mechanism, and that
fails because the new check option isn't valid against the old view
query.

So if we're going to use the ALTER VIEW mechanism to update the view's
options, which is probably the most convenient way to do it,
DefineView()/DefineVirtualRelation() will need a little re-jigging to
do things in the required order.

I'll try to knock up a patch to do that.

Regards,
Dean


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

Re: [HACKERS] PSQL commands: \quit_if, \quit_unless

[Pavel Stehule]

2016-12-17 16:26 GMT+01:00 Fabien COELHO :

>
> Hello Tom,
>
> So I think it would be reasonable for somebody to implement \if,
>>> \elseif, \endif first, with the argument having to be, precisely, a
>>> single variable and nothing else (not even a negator). [...]
>>>
>>
> This seems like a reasonable implementation plan to me, not least because
>> it tackles the hard part first.  There's no doubt that we can build an
>> expression evaluator, but I'm not entirely sure how we're going to wedge
>> conditional eval or loops into psql's command reader.
>>
>> (Or in other words, let's see \while ... \endloop in the minimal proposal
>> as well, or at least a sketch of how to get there.)
>>
>
> My 0.02 €:
>
> I have not seen any use case for a loop... Does someone have something
> convincing? I could think of some use in benchmarking (aka in pgbench), but
> not psql... But I may lack imagination.
>
> If one realistic case is found, then from a syntactic point of view
> "\while expr ... \endwhile/loop/whatever" looks straightforward enough.
>

maybe iteration over cursor can be interesting - but now with with \gexec
it is not important.


>
> However, the implementation issues are pretty different from "if" which
> can be managed pretty simply on the fly with a stack and a little
> automaton. A loop needs to store its body and evaluate it over and over,
> which means having processed the input up to the end of the loop before
> proceeding, including nesting and so... it is a much less interactive
> friendly construct.
>
> Note that although "cpp" has an if, but it does not have any loop.
>
> In my opinion psql should stay at that same simple level: ISTM that the
> typical psql-script requirement is to be able to test some things, eg for
> installing or upgrading the schema of an application, and for that
> variables, expressions server side and maybe client side, and conditions
> are mostly enough. A lot of "IF EXISTS" added to many commands recently are
> motivated to handle this kind of use-case at the command per command level,
> which is not necessarily the right place.
>
> A while loops turns a simple thing into a potential Turing-complete beast,
> without a strong incentive I think that it should be avoided.
>

+1

Pavel

>
> --
> Fabien.

Re: [HACKERS] PSQL commands: \quit_if, \quit_unless

[Fabien COELHO]

Hello Tom,


So I think it would be reasonable for somebody to implement \if,
\elseif, \endif first, with the argument having to be, precisely, a
single variable and nothing else (not even a negator). [...]



This seems like a reasonable implementation plan to me, not least because
it tackles the hard part first.  There's no doubt that we can build an
expression evaluator, but I'm not entirely sure how we're going to wedge
conditional eval or loops into psql's command reader.

(Or in other words, let's see \while ... \endloop in the minimal proposal
as well, or at least a sketch of how to get there.)


My 0.02 €:

I have not seen any use case for a loop... Does someone have something 
convincing? I could think of some use in benchmarking (aka in pgbench), 
but not psql... But I may lack imagination.


If one realistic case is found, then from a syntactic point of view 
"\while expr ... \endwhile/loop/whatever" looks straightforward enough.


However, the implementation issues are pretty different from "if" which 
can be managed pretty simply on the fly with a stack and a little 
automaton. A loop needs to store its body and evaluate it over and over, 
which means having processed the input up to the end of the loop before 
proceeding, including nesting and so... it is a much less interactive 
friendly construct.


Note that although "cpp" has an if, but it does not have any loop.

In my opinion psql should stay at that same simple level: ISTM that the 
typical psql-script requirement is to be able to test some things, eg for 
installing or upgrading the schema of an application, and for that 
variables, expressions server side and maybe client side, and conditions 
are mostly enough. A lot of "IF EXISTS" added to many commands recently 
are motivated to handle this kind of use-case at the command per command 
level, which is not necessarily the right place.


A while loops turns a simple thing into a potential Turing-complete beast, 
without a strong incentive I think that it should be avoided.


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

Re: [HACKERS] pg_basebackups and slots

[Magnus Hagander]

On Sat, Dec 17, 2016 at 3:54 PM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 12/15/16 4:04 AM, Magnus Hagander wrote:
> > This obviously requires the server to be configured with enough slots (I
> > still think we should change the default here, but that's a different
> > topic), but I think that's acceptable.
>
> We could implicitly reserve one replication slot per walsender.  And
> then max_replication_slots (possibly renamed) would just be additional
> slots beyond that.
>

That makes a lot of sense now that we have temporary replication slots, I
agree. And then max_replication_slots could be something like
persistent_replication_slots?


-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] pg_basebackups and slots

[Peter Eisentraut]

On 12/15/16 4:04 AM, Magnus Hagander wrote:
> This obviously requires the server to be configured with enough slots (I
> still think we should change the default here, but that's a different
> topic), but I think that's acceptable.

We could implicitly reserve one replication slot per walsender.  And
then max_replication_slots (possibly renamed) would just be additional
slots beyond that.

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


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

Re: [HACKERS] pg_basebackups and slots

[Magnus Hagander]

On Fri, Dec 16, 2016 at 7:40 AM, Michael Paquier 
wrote:

> On Fri, Dec 16, 2016 at 3:32 PM, Magnus Hagander 
> wrote:
> > I don't really know how to write a good test for it. I mean, we could
> run it
> > with the parameter, but how to we make it actually verify the slot? Make
> a
> > really big database to make it guaranteed to be slow enough that we can
> > notice it in a different session?
>
> No, not that. I was meaning tests to trigger the error code paths with
> the compatibility switches you are adding.
>

There is only one switch - --no-slot. I guess you mean rune one backup with
that one? Sure, that's easy enough to do. I'm not sure how much it really
tests, but let's do it :)

You mean something like this, right?

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 1f15a17..f912ed0 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -246,6 +246,20 @@ PostgreSQL documentation
  
 
  
+  --no-slot
+  
+   
+This option prevents the creation of a temporary replication slot
+during the backup even if it's supported by the server.
+   
+   
+Temporary replication slots are created by default if no slot name
+is given with the -S when using log streaming.
+   
+  
+ 
+
+ 
   -T olddir=newdir
   --tablespace-mapping=olddir=newdir
   
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index f7ba9a9..fc171c1 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -61,6 +61,11 @@ typedef struct TablespaceList
  */
 #define MINIMUM_VERSION_FOR_PG_WAL	10
 
+/*
+ * Temporary replication slots are supported from version 10.
+ */
+#define MINIMUM_VERSION_FOR_TEMP_SLOTS 10
+
 /* Global options */
 static char *basedir = NULL;
 static TablespaceList tablespace_dirs = {NULL, NULL};
@@ -79,6 +84,8 @@ static bool do_sync = true;
 static int	standby_message_timeout = 10 * 1000;		/* 10 sec = default */
 static pg_time_t last_progress_report = 0;
 static int32 maxrate = 0;		/* no limit by default */
+static char *replication_slot = NULL;
+static bool temp_replication_slot = true;
 
 static bool success = false;
 static bool made_new_pgdata = false;
@@ -323,6 +330,7 @@ usage(void)
 	printf(_("  -R, --write-recovery-conf\n"
 			 " write recovery.conf after backup\n"));
 	printf(_("  -S, --slot=SLOTNAMEreplication slot to use\n"));
+	printf(_("  --no-slot  prevent creation of temporary replication slot\n"));
 	printf(_("  -T, --tablespace-mapping=OLDDIR=NEWDIR\n"
 	  " relocate tablespace in OLDDIR to NEWDIR\n"));
 	printf(_("  -x, --xlog include required WAL files in backup (fetch mode)\n"));
@@ -452,6 +460,7 @@ typedef struct
 	char		xlog[MAXPGPATH];	/* directory or tarfile depending on mode */
 	char	   *sysidentifier;
 	int			timeline;
+	bool		temp_slot;
 } logstreamer_param;
 
 static int
@@ -471,6 +480,16 @@ LogStreamerMain(logstreamer_param *param)
 	stream.do_sync = do_sync;
 	stream.mark_done = true;
 	stream.partial_suffix = NULL;
+	stream.replication_slot = replication_slot;
+	stream.temp_slot = param->temp_slot;
+	if (stream.temp_slot && !stream.replication_slot)
+	{
+		/* Generate a name for the temporary slot */
+		char		name[32];
+
+		snprintf(name, sizeof(name), "pg_basebackup_%d", getpid());
+		stream.replication_slot = pstrdup(name);
+	}
 
 	if (format == 'p')
 		stream.walmethod = CreateWalDirectoryMethod(param->xlog, do_sync);
@@ -557,6 +576,11 @@ StartLogStreamer(char *startpos, uint32 timeline, char *sysidentifier)
 			 PQserverVersion(conn) < MINIMUM_VERSION_FOR_PG_WAL ?
 "pg_xlog" : "pg_wal");
 
+	/* Temporary replication slots are only supported in 10 and newer */
+	if (PQserverVersion(conn) < MINIMUM_VERSION_FOR_TEMP_SLOTS)
+		param->temp_slot = false;
+	else
+		param->temp_slot = temp_replication_slot;
 
 	if (format == 'p')
 	{
@@ -2052,11 +2076,13 @@ main(int argc, char **argv)
 		{"verbose", no_argument, NULL, 'v'},
 		{"progress", no_argument, NULL, 'P'},
 		{"xlogdir", required_argument, NULL, 1},
+		{"no-slot", no_argument, NULL, 2},
 		{NULL, 0, NULL, 0}
 	};
 	int			c;
 
 	int			option_index;
+	bool		no_slot = false;
 
 	progname = get_progname(argv[0]);
 	set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_basebackup"));
@@ -2106,7 +2132,16 @@ main(int argc, char **argv)
 writerecoveryconf = true;
 break;
 			case 'S':
+
+/*
+ * When specifying replication slot name, use a permanent
+ * slot.
+ */
 replication_slot = pg_strdup(optarg);
+temp_replication_slot = false;
+break;
+			case 2:
+no_slot = true;
 break;
 			case 'T':
 tablespace_list_append(optarg);
@@ -2268,7 

Re: [HACKERS] pg_basebackups and slots

[Magnus Hagander]

On Fri, Dec 16, 2016 at 12:41 PM, Petr Jelinek  wrote:

> On 16/12/16 07:32, Magnus Hagander wrote:
> >
> > On Dec 16, 2016 07:27, "Michael Paquier"  > > wrote:
> >
> >
> >
> > On Thu, Dec 15, 2016 at 7:28 PM, Magnus Hagander
> > > wrote:
> > > So here's a patch that does this, for discussion. It implements the
> > > following behavior for -X:
> > >
> > > * When used with <10.0 servers, behave just like before.
> > > * When -S  is specified, behave just like before (use an
> > existing
> > > replication slot, fail if it does not exist)
> > > * When used on 10.0 with no -S, create and use a temporary
> > replication slot
> > > while running, with name pg_basebackup_.
> > > * When used with 10.0 with no -S but --no-slot specified, run
> > without a slot
> > > like before.
> >
> > There are users using -X stream without a slot now because they
> > don't want to
> > cause WAL retention in pg_xlog and are ready for retries in taking
> > the base
> > backup... I am wondering if it is a good idea to change the default
> > behavior
> > and not introduce a new option like --temporary-slot, or
> > --slot-mode=temp|persistent|none with none being the default
> > instead. There
> > are users caring about pg_xlog not filling up if pg_basebackup hangs
> > on write
> > for example. And it may be a good idea to be able to use
> > --slot-mode=temp
> > with -S/--slot actually to allow users to monitor it. If no slot
> > name is given
> > with --slot generating one looks fine to me.
> >
> >
> > I really think the default should be "what most people want", and not
> > "whatever is compatible with a mode that was necessary because we lacked
> > infrastructure".
>
> +1 (btw glad you made the patch)
>

I'm glad you made the temp slots, so I could abandon that branch :)



> > Yes there are some people who are fine with their backups failing. I'm
> > willing to say there are *many* more who benefit from an automatic slot.
> >
>
> I think the issue with slots taking over disk space should be fixed by
> making it possible to cut off slots when necessary (ie some GUC that
> would say how much behind slot can be at maximum, with something like 0
> being unlimited like it's now) instead of trying to work around that in
> every client.
>

Agreed, that seems like a better solution.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] [PATCH] Remove trailing whitespaces from documentation

[Peter Eisentraut]

On 12/14/16 6:10 PM, Vladimir Rusinov wrote:
> Either way, I've attached another version of my patch - this time it
> avoids touching example psql output. Baby steps.
> I'll let you decide on the way forward. I'm just happy to send some patches.

committed

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


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

Re: [HACKERS] Make pg_basebackup -x stream the default

[Magnus Hagander]

On Fri, Dec 16, 2016 at 6:35 PM, Fujii Masao  wrote:

> On Fri, Dec 16, 2016 at 11:36 PM, Magnus Hagander 
> wrote:
> > On Thu, Dec 15, 2016 at 12:37 AM, Vladimir Rusinov 
> > wrote:
> >>
> >> Usability review
> >>
> >> 
> >>
> >>
> >> Patch sounds like a good idea and does what it supposed to do. /me in
> DBA
> >> hat will be happy to have it.
> >>
> >> However, it makes '-x' parameter a bit confusing/surprising: specifying
> it
> >> will be equivalent to '-X fetch' which is surprising regression from
> the new
> >> default.
> >
> >
> > This seems like a good idea, really.
> >
> > Given that we already break a number of other things around backups and
> > replication in this release, it seems like a good time.
> >
> > I definitely think removing it is what we should do -- let's not
> redefine it
> > to mean streaming, let's just get rid of -x altogether, and have people
> use
> > -X streaming|fetch|none.
> >
> > What do others feel about this?
>
> +1 to drop -x option. That's less confusing.
>

Attached is an updated patch that does this. As a bonus it simplifies the
code a bit. I also fixed an error message that I missed updating in the
previous patch.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 1f15a17..b6381ea 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -88,7 +88,7 @@ PostgreSQL documentation
   There is no guarantee that all WAL files required for the backup are archived
   at the end of backup. If you are planning to use the backup for an archive
   recovery and want to ensure that all required files are available at that moment,
-  you need to include them into the backup by using the -x option.
+  you need to include them into the backup by using the -X option.
  
 
 
@@ -285,27 +285,16 @@ PostgreSQL documentation
  
 
  
-  -x
-  --xlog
-  
-   
-Using this option is equivalent of using -X with
-method fetch.
-   
-  
- 
-
- 
   -X method
   --xlog-method=method
   

 Includes the required transaction log files (WAL files) in the
 backup. This will include all transaction logs generated during
-the backup. If this option is specified, it is possible to start
-a postmaster directly in the extracted directory without the need
-to consult the log archive, thus making this a completely standalone
-backup.
+the backup. Unless the method none is specified,
+it is possible to start a postmaster directly in the extracted
+directory without the need to consult the log archive, thus
+making this a completely standalone backup.


 The following methods for collecting the transaction logs are
@@ -313,6 +302,16 @@ PostgreSQL documentation
 
 
  
+  n
+  none
+  
+   
+Don't include transaction log in the backup.
+   
+  
+ 
+
+ 
   f
   fetch
   
@@ -349,6 +348,9 @@ PostgreSQL documentation
 named pg_wal.tar (if the server is a version
 earlier than 10, the file will be named pg_xlog.tar).

+   
+This value is the default.
+   
   
  
 
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index f7ba9a9..459f36d 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -71,8 +71,8 @@ static bool noclean = false;
 static bool showprogress = false;
 static int	verbose = 0;
 static int	compresslevel = 0;
-static bool includewal = false;
-static bool streamwal = false;
+static bool includewal = true;
+static bool streamwal = true;
 static bool fastcheckpoint = false;
 static bool writerecoveryconf = false;
 static bool do_sync = true;
@@ -325,8 +325,7 @@ usage(void)
 	printf(_("  -S, --slot=SLOTNAMEreplication slot to use\n"));
 	printf(_("  -T, --tablespace-mapping=OLDDIR=NEWDIR\n"
 	  " relocate tablespace in OLDDIR to NEWDIR\n"));
-	printf(_("  -x, --xlog include required WAL files in backup (fetch mode)\n"));
-	printf(_("  -X, --xlog-method=fetch|stream\n"
+	printf(_("  -X, --xlog-method=none|fetch|stream\n"
 			 " include required WAL files with specified method\n"));
 	printf(_("  --xlogdir=XLOGDIR  location for the transaction log directory\n"));
 	printf(_("  -z, --gzip compress tar output\n"));
@@ -1700,7 +1699,11 @@ BaseBackup(void)
 	 */
 	if (streamwal && !CheckServerVersionForStreaming(conn))
 	{
-		/* Error message already written in CheckServerVersionForStreaming() */
+		/*
+	

Re: [HACKERS] Proposal for changes to recovery.conf API

[Magnus Hagander]

On Sat, Dec 17, 2016 at 5:42 AM, Bruce Momjian  wrote:

> On Fri, Dec 16, 2016 at 08:52:25PM -0500, Robert Haas wrote:
> > On Fri, Dec 16, 2016 at 8:07 PM, Tom Lane  wrote:
> > > I'm still not seeing any value in putting this sort of info into
> > > a documentation section that's distinct from the release notes.
> > > We've used links to wiki pages in the past when the information
> > > seemed to be in flux, and that's reasonable.  But what's the point
> > > of just linking to somewhere else in the same document?
> >
> > If the explanation is just a few sentences long, I see no reason not
> > to include it in the release notes.  But if it's comparable in length
> > to a moderately-long chapter then why would we not make it its own
> > chapter?  I think your argument boils down to "people probably don't
> > need very much detail about this".  But I think that's the wrong line
> > of thinking.  In my view, we ought to ship just about as much quality
> > documentation as people are willing to write.  Saying that we're going
> > to reject good-quality documentation because we don't want to have too
> > much of it is like saying we want to reject good-quality features
> > because we don't want to have too many of them, or good-quality
> > regression tests because we don't want to have too much code coverage,
>
> [ I stopped reading after this. ]
>
> The point is that the documentation about the recovery.conf changes in
> Postgres are only interesting to people migrating to Postgres 10, i.e.
> this is not quality documentation for someone going from Postgres 10 to
> Postgres 11.
>
>
They will also be interesting to people going from 9.4 to 11, or from 9.3
to 12. Or from 9.5 to 13.

They only become uninteresting when we stop supporting 9.6 which is the
last version that didn't have that functionality.


-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] Typos in waldender.c and slot.c

[Magnus Hagander]

On Sat, Dec 17, 2016 at 2:08 PM, Michael Paquier 
wrote:

> Hi all,
>
> While going through the temporary replication slot commits, I have
> noticed two typos as attached.
>

Applied, thanks.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

[HACKERS] Typos in waldender.c and slot.c

[Michael Paquier]

Hi all,

While going through the temporary replication slot commits, I have
noticed two typos as attached.
Thanks,
-- 
Michael


slot-typos.patch
Description: application/download

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

Re: [HACKERS] Logical Replication WIP

[Erik Rijkers]

On 2016-12-16 13:49, Petr Jelinek wrote:


version 13 of the patch.

0001-Add-PUBLICATION-catalogs-and-DDL-v13.patch.gz (~32 KB)
0002-Add-SUBSCRIPTION-catalog-and-DDL-v13.patch.gz (~28 KB)
0003-Define-logical-rep...utput-plugi-v13.patch.gz (~13 KB)
0004-Add-logical-replication-workers-v13.patch.gz (~44 KB)
0005-Add-separate-synch...or-logical--v13.patch.gz (~2 KB)


Hi,

You wrote on 2016-08-05: :


What's missing:
 - sequences, I'd like to have them in 10.0 but I don't have good
   way to implement it. PGLogical uses periodical syncing with some
   buffer value but that's suboptimal. I would like to decode them
   but that has proven to be complicated due to their sometimes
   transactional sometimes nontransactional nature, so I probably
   won't have time to do it within 10.0 by myself.


I ran into problems with sequences and I wonder if sequences-problems
are still expected, as the above seems to imply.

(short story: I tried to run pgbench across logical replication; and 
therefore

added a sequence to pgbench_history to give it a replica identity, and
cannot get it to work reliably ).


thanks,

Eik Rijkers







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

Re: [HACKERS] Crash on promotion when recovery.conf is renamed

[Magnus Hagander]

On Fri, Dec 16, 2016 at 7:08 AM, Michael Paquier 
wrote:

> On Thu, Dec 15, 2016 at 11:25:10AM +0200, Heikki Linnakangas wrote:
> > On 12/15/2016 10:44 AM, Magnus Hagander wrote:
> > > I wonder if there might be more corner cases like this, but in this
> > > particular one it seems easy enough to just say that failing to rename
> > > recovery.conf because it didn't exist is safe.
> >
> > Yeah. It's unexpected though, so I think erroring out is quite
> reasonable.
> > If the recovery.conf file went missing, who knows what else is wrong.
>
> i'd rather not mess up with the exiting behavior and just complain to the
> pilot to not do that. This enters in the category of not removing the
> backup_label file after taking a backup...
>

I'm happy to say that people shouldn't do that. However, we have a system
that is unnecessarily fragile, by making something like that so easy to
break. This could equally happen in other ways. For example, someone could
accidentally provision a recovery.conf with the wrong permissions.

Reducing the fragility of such an important part of the system is a big
improvement, IMO. Of course, we have to be extra careful when touching
those parts.

Bottom line, I'm perfectly fine with failing in such a scenario. I'm not
fine with leaving the user with a corrupt cluster if we can avoid it.


As for your comparison, we fixed the backup_label part by adding support
for taking backups without using the fragile system. So it's not like we
didn't recognize the problem.



>
> > > But in the case of failing to rename recovery.conf for example because
> of
> > > permissions errors, we don't want to ignore it. But we also really
> don't
> > > want to end up with this kind of inconsistent data directory IMO. I
> don't
> > > know that code well enough to suggest how to fix it though -- hoping
> for
> > > input for someone who knows it closer?
> >
> > Hmm. Perhaps we should write the timeline history file only after
> renaming
> > recovery.conf. In general, we should keep the window between writing the
> > timeline history file and writing the end-of-recovery record as small as
> > possible. I don't think we can eliminate it completely, but it makes
> sense
> > to minimize it. Something like the attached (completely untested).
>

I haven't looked at the code either, but the reason is definitely solid -
let's keep the time as short as possible. In particular, keeping it
recoverable for things that are caused by humans, because they will keep
making mistakes. And editing recovery.conf is a normal thing for a DBA to
do (sure, not removing it -- but it's one of the few files in the data
directory that the DBA is actually supposed to handle manually, so that
increases the risk).

And also by doing things in an order that will at least make it less likely
to end up corrupt if people manage to do it anyway.


I did some tests. And after a lookup it looks like a good thing to book
> the timeline number at an earlier step and let other nodes know about
> it. So +1 on it.
>


>
> Looking at PrescanPreparedTransactions(), I am thinking as well that it
> would
> be better to get a hard failure when bumping on a corrupted 2PC file.
> Future
> files are one thing, but corrupted files should be treated more carefully.
>

Again without looking at it, I agree (so much easier that way :P). Ignoring
corruption is generally a bad idea. Failing hard makes the user notice the
error, and makes it possible to initiate recovery from a standby or from
backups or something, or to *intentionally* remove/clear/ignore it.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] Hang in pldebugger after git commit : 98a64d0

[Ashutosh Sharma]

> It is fine as per current usage of WaitEventSet API's, however,
> Robert's point is valid that user is not obliged to call
> ModifyWaitEvent before WaitEventSetWait.  Imagine a case where some
> new user of this API is calling WaitEventSetWait repeatedly without
> calling ModifyWaitEvent.

Oops! I never thought this scenario, yes there can be a case where an
external module like FDW may call WaitEventSetWait() and in such case
the fix provided in v1 patch may not work. So, yes we may have to
reset the event in WaitEventSetWaitBlock(). I am extremely extremely
sorry for a noise.


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

Re: [HACKERS] Hang in pldebugger after git commit : 98a64d0

[Amit Kapila]

On Sat, Dec 17, 2016 at 3:53 PM, Ashutosh Sharma  wrote:
> Hi,
>
>>>  I think it should be the responsibility of
>>> WaitEventSetWaitBlock() to reset the event, if needed, before calling
>>> WaitForMultipleObjects().
>>>
>>
>> If we want to change WaitEventSetWaitBlock then ideally we need to
>> change all other wait API's (WAIT_USE_SELECT,  WAIT_USE_POLL, etc.) as
>> well.  I don't see the need for it, can't we do it in
>> WaitEventSetWait() after processing all the returned events.  It can
>> be done by enumerating all wait events and reset the events for which
>> reset flag is true.  If we see code prior to 9.6, this event (socket
>> event) has been reset only after processing all returned events, not
>> after every event.
>
> I think we are trying the complicate the things, I would look into the
> fix like this, Let us assume that WaitForMultipleObjects() says an
> event occured on a LATCH then we would save the occured event and
> reset the LATCH immediately inside WaitEventSetWaitBlock() similarly
> if event occurs on SOCKET we should save the occured events and reset
> it. Now, the only concern here is why can't we reset event on SOCKET
> using ResetEvent() API instead of introducing a new variable. I think
> that is just because we are trying to avoid the events for SOCKET from
> being re-created again and again. So, for me i think Amit's fix is
> absolutely fine and is restricted to Windows.
>

It is fine as per current usage of WaitEventSet API's, however,
Robert's point is valid that user is not obliged to call
ModifyWaitEvent before WaitEventSetWait.  Imagine a case where some
new user of this API is calling WaitEventSetWait repeatedly without
calling ModifyWaitEvent.


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


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

Re: [HACKERS] Hang in pldebugger after git commit : 98a64d0

[Amit Kapila]

On Sat, Dec 17, 2016 at 9:34 AM, Robert Haas  wrote:
> On Fri, Dec 16, 2016 at 10:34 PM, Amit Kapila  wrote:
>>>  I think it should be the responsibility of
>>> WaitEventSetWaitBlock() to reset the event, if needed, before calling
>>> WaitForMultipleObjects().
>>>
>>
>> If we want to change WaitEventSetWaitBlock then ideally we need to
>> change all other wait API's (WAIT_USE_SELECT,  WAIT_USE_POLL, etc.) as
>> well.
>
> Why?  This is only broken on Windows.  It would be nicer not to touch
> any of the un-broken implementations.
>

Yeah, but we are planning to add a generic flag in WaitEvent structure
which can be used for similar purpose.  However, as per your
suggestion, I have changed it only for Win32 port.  Also, I kept the
change in ModifyWaitEvent API as that seems to be okay considering
'reset' is a generic flag in WaitEvent structure.

>>>  BTW, I suggest this rewritten comment:
>>>
>>> /*--
>>>  * FD_READ events are edge-triggered on Windows per
>>>  * 
>>> https://msdn.microsoft.com/en-us/library/windows/desktop/ms741576(v=vs.85).aspx
>>>
>>
>> Isn't the statement in above doc. "For FD_READ, FD_OOB, and FD_ACCEPT
>> network events, network event recording and event object signaling are
>> level-triggered." says that FD_READ events are level-triggered which
>> seems to be contradictory with above comment?
>
> Argh.  I see your point.  Maybe we'd better rephrase that.  The
> document does say that, but the behavior they described is actually a
> weird hybrid of level-triggered and edge-triggered.  We should
> probably just avoid that terminology altogether and explain that
> read-ready won't necessarily be re-signalled unless there is an
> intervening recv().
>

I also think so.  I have modified your suggested comment in that way.


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


reset_wait_events_v2.patch
Description: Binary data

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

Re: [HACKERS] Hang in pldebugger after git commit : 98a64d0

[Ashutosh Sharma]

Hi,

>>  I think it should be the responsibility of
>> WaitEventSetWaitBlock() to reset the event, if needed, before calling
>> WaitForMultipleObjects().
>>
>
> If we want to change WaitEventSetWaitBlock then ideally we need to
> change all other wait API's (WAIT_USE_SELECT,  WAIT_USE_POLL, etc.) as
> well.  I don't see the need for it, can't we do it in
> WaitEventSetWait() after processing all the returned events.  It can
> be done by enumerating all wait events and reset the events for which
> reset flag is true.  If we see code prior to 9.6, this event (socket
> event) has been reset only after processing all returned events, not
> after every event.

I think we are trying the complicate the things, I would look into the
fix like this, Let us assume that WaitForMultipleObjects() says an
event occured on a LATCH then we would save the occured event and
reset the LATCH immediately inside WaitEventSetWaitBlock() similarly
if event occurs on SOCKET we should save the occured events and reset
it. Now, the only concern here is why can't we reset event on SOCKET
using ResetEvent() API instead of introducing a new variable. I think
that is just because we are trying to avoid the events for SOCKET from
being re-created again and again. So, for me i think Amit's fix is
absolutely fine and is restricted to Windows. Please do correct me if
my point is wrong. Thank you.

With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com


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

[HACKERS] too low cost of Bitmap index scan

[Pavel Stehule]

Hi

I am trying to fix slow query on PostgreSQL 9.5.4.

The data are almost in RAM

I have a problem with too low cost slow Bitmap index scan on date column,
that returns 300K rows.

Slow part
->  Bitmap Heap Scan on "Zasilka"  (cost=5097.39..5670.64 rows=1 width=12)
(actual time=62.253..62.400 rows=3 loops=231)
 Recheck Cond: (("Dopravce" = "Dopravce_Ridic_1"."ID") AND ("StavDatum"
> (now() - '10 days'::interval)))
 Filter: (("Stav" = 34) OR ("Stav" = 29) OR ("Stav" = 180) OR ("Stav" =
213) OR ("Stav" = 46) OR (("Produkt" = 33) AND ("Stav" = 179)) OR
((("ZpetnaZasilka" = '-1'::integer) OR ("PrimaZasilka" = '-1'::integer))
AND ("Stav" = 40)))
 Rows Removed by Filter: 154
 Heap Blocks: exact=22038
  ->  BitmapAnd  (cost=5097.39..5097.39 rows=144 width=0) (actual
time=61.725..61.725 rows=0 loops=231)
   ->  Bitmap Index Scan on "Zasilka_idx_Dopravce"
(cost=0.00..134.05 rows=7594 width=0) (actual time=1.030..1.030 rows=7608
loops=231)
 Index Cond: ("Dopravce" = "Dopravce_Ridic_1"."ID")
   ->  Bitmap Index Scan on "Zasilka_idx_StavDatum"
(cost=0.00..4963.34 rows=290487 width=0) (actual time=65.505..65.505
rows=354423 loops=210)
 Index Cond: ("StavDatum" > (now() - '10
days'::interval))

When I disable bitmap scan, then the query is 6x time faster

   ->  Index Scan using "Dopravce_Ridic_idx_Kod" on "Dopravce_Ridic"
"Dopravce_Ridic_1"  (cost=0.00..8.02 rows=1 width=4) (actual
time=0.008..0.017 rows=1 loops=308)
 Index Cond: (("Kod")::text = ("Dopravce_Ridic"."Kod")::text)
 Filter: (substr(("Kod")::text, 1, 1) <> 'S'::text)
 Rows Removed by Filter: 0
   ->  Index Scan using "Zasilka_idx_Dopravce" on "Zasilka"
(cost=0.00..30489.04 rows=1 width=12) (actual time=15.651..17.187 rows=3
loops=231)
Index Cond: ("Dopravce" = "Dopravce_Ridic_1"."ID")
   Filter: (("StavDatum" > (now() - '10 days'::interval)) AND (("Stav"
= 34) OR ("Stav" = 29) OR ("Stav" = 180) OR ("Stav" = 213) OR ("Stav" = 46)
OR (("Produkt" = 33) AND ("Stav" = 179)) OR ((("ZpetnaZasilka" =
'-1'::integer) OR ("PrimaZasilka" = '-1'::integer)) AND ("Stav" = 40
Rows Removed by Filter: 7596

I tested composite index ("Dopravce", "StavDatum"), but without success -
planner still prefer bitmap index scan.

Table "Zasilka" is big with 15GB data

Regards

Pavel