Re: [HACKERS] [sqlsmith] Failed assertion in joinrels.c

[Andreas Seltenreich]

Peter Geoghegan writes:

> On Fri, Jul 31, 2015 at 5:56 PM, Andreas Seltenreich  
> wrote:
>> sqlsmith triggered the following assertion in master (c188204).
>
> Thanks for writing sqlsmith. It seems like a great tool.
>
> I wonder, are you just running the tool with assertions enabled when
> PostgreSQL is built?

Right.  I have to admit my testing setup is still more tailored towards
testing sqlsmith than postgres.

> If so, it might make sense to make various problems more readily
> detected.  As you may know, Clang has a pretty decent option called
> AddressSanitizer that can detect memory errors as they occur with an
> overhead that is not excessive.

I didn't known this clang feature yet, thanks for pointing it out.  I
considered running some instances under valgrind to detect these, but
the performance penalty seemed not worth it.

> One might use the following configure arguments when building
> PostgreSQL to use AddressSanitizer:
>
> ./configure CC=clang CFLAGS='-O1 -g -fsanitize=address
> -fno-omit-frame-pointer -fno-optimize-sibling-calls' --enable-cassert

A quick attempt to sneak these in made my ansible playbooks unhappy due
to "make check" failures and other generated noise.  I'll try to have an
instance with the AddressSanitizer active soon though.

> Of course, it remains to be seen if this pays for itself. Apparently
> the tool has about a 2x overhead [1]. I'm really not sure that you'll
> find any more bugs this way, but it's certainly possible that you'll
> find a lot more. Given your success in finding bugs without using
> AddressSanitizer, introducing it may be premature.

Piotr also suggested on IRC to run coverage tests w/ sqlsmith.  This
could yield valuable hints in which direction to extend sqlsmith's
grammar.

Thanks,
Andreas


-- 
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] Tab completion for CREATE SEQUENCE

[Michael Paquier]

On Tue, Jul 7, 2015 at 9:14 PM, Andres Freund  wrote:
> On 2015-06-19 06:41:19 +, Brendan Jurd wrote:
>> I'm marking this "Waiting on Author".  Once the problems have been
>> corrected, it should be ready for a committer.
>
> Vik, are you going to update the patch?

Seeing no activity on this thread and as it would be a waste not to do
it, I completed the patch as attached. The following things are done:
- Fixed code indentation
- Removal of "RESTART", "SET SCHEMA", "OWNER TO", and "RENAME TO" in
CREATE SEQUENCE
- Added "START WITH".
- Added TEMP/TEMPORARY in the set of keywords tracked.
I am switching at the same time this patch as "Ready for committer".
Regards,
-- 
Michael
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index ece0515..0748284 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2467,6 +2467,35 @@ psql_completion(const char *text, int start, int end)
 			 pg_strcasecmp(prev_wd, "TO") == 0)
 		COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables, NULL);
 
+/* CREATE TEMP/TEMPORARY SEQUENCE  */
+	else if ((pg_strcasecmp(prev3_wd, "CREATE") == 0 &&
+			  pg_strcasecmp(prev2_wd, "SEQUENCE") == 0) ||
+			 (pg_strcasecmp(prev4_wd, "CREATE") == 0 &&
+			  (pg_strcasecmp(prev3_wd, "TEMP") == 0 ||
+			   pg_strcasecmp(prev3_wd, "TEMPORARY") == 0) &&
+			  pg_strcasecmp(prev2_wd, "SEQUENCE") == 0))
+	{
+		static const char *const list_CREATESEQUENCE[] =
+		{"INCREMENT BY", "MINVALUE", "MAXVALUE", "RESTART", "NO", "CACHE",
+		 "CYCLE", "OWNED BY", "START WITH", NULL};
+
+		COMPLETE_WITH_LIST(list_CREATESEQUENCE);
+	}
+/* CREATE TEMP/TEMPORARY SEQUENCE  NO */
+	else if ((pg_strcasecmp(prev4_wd, "CREATE") == 0 &&
+			  pg_strcasecmp(prev3_wd, "SEQUENCE") == 0) ||
+			 (pg_strcasecmp(prev5_wd, "CREATE") == 0 &&
+			  (pg_strcasecmp(prev4_wd, "TEMP") == 0 ||
+			   pg_strcasecmp(prev4_wd, "TEMPORARY") == 0) &&
+			  pg_strcasecmp(prev3_wd, "SEQUENCE") == 0) &&
+			 pg_strcasecmp(prev_wd, "NO") == 0)
+	{
+		static const char *const list_CREATESEQUENCE2[] =
+		{"MINVALUE", "MAXVALUE", "CYCLE", NULL};
+
+		COMPLETE_WITH_LIST(list_CREATESEQUENCE2);
+	}
+
 /* CREATE SERVER  */
 	else if (pg_strcasecmp(prev3_wd, "CREATE") == 0 &&
 			 pg_strcasecmp(prev2_wd, "SERVER") == 0)

-- 
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] Sharing aggregate states between different aggregate functions

[David Rowley]

On 29 July 2015 at 03:45, Heikki Linnakangas  wrote:

> On 07/28/2015 04:14 AM, David Rowley wrote:
>
>> On 27 July 2015 at 20:11, Heikki Linnakangas  wrote:
>>
>> On 07/27/2015 08:34 AM, David Rowley wrote:
>>>
>>> In this function I also wasn't quite sure if it was with comparing both
 non-NULL INITCOND's here. I believe my code comments may slightly
 contradict what the code actually does, as the comments talk about them
 having to match, but the code just bails if any are non-NULL. The
 reason I
 didn't check them was because it seems inevitable that some duplicate
 work
 needs to be done when setting up the INITCOND. Perhaps it's worth it?

>>>
>>> It would be nice to handle non-NULL initconds. I think you'll have to
>>> check that the input function isn't volatile. Or perhaps just call the
>>> input function, and check that the resulting Datum is byte-per-byte
>>> identical, although that might be awkward to do with the current code
>>> structure.
>>>
>>
>> I've not done anything with this.
>> I'd not thought of an input function being volatile before, but I guess
>> it's possible, which makes me a bit scared that we could be treading on
>> ground we shouldn't be. I know it's more of an output function thing than
>> an input function thing, but a GUC like extra_float_digits could cause
>> problems here.
>>
>
> Yeah, a volatile input function seems highly unlikely, but who knows. BTW,
> we're also not checking if the transition or final functions are volatile.
> But that was the same before this patch too.
>
> It sure would be nice to support the built-in float aggregates, so I took
> a stab at this. I heavily restructured the code again, so that there are
> now two separate steps. First, we check for any identical Aggrefs that
> could be shared. If that fails, we proceed to the permission checks, look
> up the transition function and build the initial datum. And then we call
> another function that tries to find an existing, compatible per-trans
> structure. I think this actually looks better than before, and checking for
> identical init values is now easy. This does lose one optimization: if
> there are two aggregates with identical transition functions and final
> functions, they are not merged into a single per-Agg struct. They still
> share the same per-Trans struct, though, and I think that's enough.
>
> How does the attached patch look to you? The comments still need some
> cleanup, in particular, the explanations of the different scenarios don't
> belong where they are anymore.
>

I've read over the patch and you've managed to implement the init value
checking much more cleanly than I had imagined it to be.
I like the 2 stage checking.

Attached is a delta patched which is based
on sharing_aggstate-heikki-2.patch to fix up the out-dated comments and
also a few more test scenarios which test the sharing works with matching
INITCOND and that it does not when they don't match.

What do you think?


>
> BTW, the permission checks were not correct before. You cannot skip the
> check on the transition function when you're sharing the per-trans state.
> We check that the aggregate's owner has permission to execute the
> transition function, and the previous aggregate whose state value we're
> sharing might have different owner.


oops, thank for noticing that and fixing.

Regards

David Rowley

 --
 David Rowley   http://www.2ndQuadrant.com/

 PostgreSQL Development, 24x7 Support, Training & Services


sharing_aggstate-heikki-2_delta1.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] pageinspect patch, for showing tuple data

[Michael Paquier]

On Mon, Aug 3, 2015 at 1:03 AM, Nikolay Shaplov
 wrote:
> Hi!
>
> I've created a patch for pageinspect that allows to see data stored in the
> tuple.
>
> This patch has two main purposes:
>
> 1. Practical: Make manual DB recovery more simple

To what are you referring to in this case? Manual manipulation of
on-disk data manually?

> b) I have plenty of sanity check while reading parsing that tuple, for this
> function I've changed error level from ERROR to WARNING. This function will
> allow to write proper tests that all these checks work as they were designed
> (I hope to write these tests sooner or later)

+ereport(inter_call_data->error_level,
+(errcode(ERRCODE_DATA_CORRUPTED),
+errmsg("Data corruption: Iterating over
tuple data reached out of actual tuple size")));
I don't think that the possibility to raise a WARNING is a good thing
in those code paths. If data is corrupted this may crash, and I am not
sure that anybody would want that even for educational purposes.
-- 
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] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

[Michael Paquier]

On Sat, Aug 1, 2015 at 9:20 PM, Andres Freund wrote:
> On August 1, 2015 2:17:24 PM GMT+02:00, Michael Paquier wrote:
>>> For instance, if you told me to choose between ShareLock and
>>> ShareUpdateExclusiveLock I wouldn't know which one is strongest.  I
>>> don't it's sensible to have the "lock mode compare" primitive
>>honestly.
>>> I don't have any great ideas to offer ATM sadly.
>>
>>Yes, the thing is that lowering the lock levels is good for
>>concurrency, but the non-monotony of the lock levels makes it
>>impossible to choose an intermediate state correctly.
>
> How about simply acquiring all the locks individually of they're different 
> types? These few acquisitions won't matter.

As long as this only applies on master, this may be fine... We could
basically pass a LOCKMASK to the multiple layers of tablecmds.c
instead of LOCKMODE to track all the locks that need to be taken, and
all the relations open during operations. Would it be worth having
some routines like relation_multi_[open|close]() as well? Like that:
Relation[] relation_multi_open(relation Oid, LOCKMASK):
void relation_multi_close(Relation[]);

If we do something, we may consider patching as well 9.5, it seems to
me that tablecmds.c is broken by assuming that lock hierarchy is
monotone in AlterTableGetLockLevel().
-- 
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] Autonomous Transaction is back

[Rajeev rastogi]

On 31 July 2015 23:10, Robert Haas Wrote: 
On Tue, Jul 28, 2015 at 6:01 AM, Craig Ringer  wrote:
>> That should be practical to special-case by maintaining a list of 
>> parent transaction (virtual?) transaction IDs. Attempts to wait on a 
>> lock held by any of those should fail immediately. There's no point 
>> waiting for the deadlock detector since the outer tx can never 
>> progress and commit/rollback to release locks, and it might not be 
>> able to see the parent/child relationship from outside the backend 
>> doing the nested tx anyway.

>I think we're going entirely down the wrong path here.  Why is it ever useful 
>for a backend's lock requests to conflict with themselves, even with 
>autonomous transactions?  That seems like an artifact of somebody else's 
>implementation that we should be happy we don't need to copy.

IMHO, since most of the locking are managed at transaction level not backend 
level and we consider main & autonomous transaction to be independent 
transaction, then practically they may conflict right.   
It is also right as you said that there is no as such useful use-cases where 
autonomous transaction conflicts with main (parent) transaction. But we cannot 
take it for granted as user might make a mistake. So at-least we should have 
some mechanism to handle this rare case, for which as of now I think throwing 
error from autonomous transaction as one of the solution. Once error thrown 
from autonomous transaction, main transaction may continue as it is (or abort 
main transaction also??). 

Any other suggestion to handle this will be really helpful.

Thanks and Regards,
Kumar Rajeev Rastogi




-- 
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] Transactions involving multiple postgres foreign servers

[Amit Kapila]

On Thu, Jul 9, 2015 at 3:48 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:
>
> 2. New catalog - This method takes out the need to have separate method for
>>> C1, C5 and even C2, also the synchronization will be taken care of by row
>>> locks, there will be no limit on the number of foreign transactions as
>>> well
>>> as the size of foreign prepared transaction information. But big problem
>>> with this approach is that, the changes to the catalogs are atomic with
>>> the
>>> local transaction. If a foreign prepared transaction can not be aborted
>>> while the local transaction is rolled back, that entry needs to retained.
>>> But since the local transaction is aborting the corresponding catalog
>>> entry
>>> would become invisible and thus unavailable to the resolver (alas! we do
>>> not have autonomous transaction support). We may be able to overcome
>>> this,
>>> by simulating autonomous transaction through a background worker (which
>>> can
>>> also act as a resolver). But the amount of communication and
>>> synchronization, might affect the performance.
>>>
>>
>> Or you could insert/update the rows in the catalog with xmin=FrozenXid,
>> ignoring MVCC. Not sure how well that would work.
>>
>
> I am not aware how to do that. Do we have any precedence in the code.
> Something like a reference implementation, which I can follow.
>

Does some thing on lines of Copy Freeze can help here?

However if you are going to follow this method, then I think you
need to also ensure when and how to clear those rows after
rollback is complete or once resolver has resolved those prepared
foreign transactions.



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

Re: [HACKERS] buffer locking fix for lazy_scan_heap

[高增琦]

Hi,

sorry for asking this really old commit.

http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=7ab9b2f3b79177e501a1ef90ed004cc68788abaf

I could not understand why "releases the lock on the buffer" is
a problem since vacuum will lock and check the page bit again before
set the vm. Did I missed something?

Thanks for your help.

On Thu, Apr 19, 2012 at 4:09 AM, Robert Haas  wrote:

> I discovered when researching the issue of index-only scans and Hot
> Standby that there's a bug (for which I'm responsible) in
> lazy_scan_heap[1].  Actually, the code has been like this for a long
> time, but I needed to change it when I did the crash-safe visibility
> map work, and I didn't.  The problem is that lazy_scan_heap() releases
> the lock on the buffer it's whacking around before it sets the
> visibility map bit.  Thus, it's possible for the page-level bit to be
> cleared by some other backend before the visibility map bit gets set.
> In previous releases that was arguably OK, since the worst thing that
> could happen is a postponement of vacuuming on that page until the
> next anti-wraparound cycle; but now that we have index-only scans, it
> can cause queries to return wrong answers.
>
> Attached is a patch to fix the problem, which rearranges things so
> that we set the bit in the visibility map before releasing the buffer
> lock.  Similar work has already been done for inserts, updates, and
> deletes, which in previous releases would *clear* the visibility map
> bit after releasing the page lock, and no longer done.  But the vacuum
> case, which *sets* the bit, was overlooked.  Our convention in those
> related cases is that it's acceptable to hold the buffer lock while
> setting the visibility map bit and issuing the WAL log record, but
> there must be no possibility of an I/O to read in the visibility map
> page while the buffer lock is held.  This turned out to be pretty
> simple because, as it turns out, lazy_scan_heap() is almost always
> holding a pin on the correct page anyway, so I only had to tweak
> things slightly to guarantee it.  As a pleasant side effect, the new
> code is actually quite a bit simpler and more readable than the old
> code, at least IMHO.
>
> While I was at it, I made a couple of minor, related changes which I
> believe to be improvements.  First, I arranged things so that, when we
> pause the first vacuum pass to do an index vac cycle, we release any
> buffer pin we're holding on the visibility map page.  The fact that we
> haven't done that in the past is probably harmless, but I think
> there's something to be said for not holding onto buffer pins for long
> periods of time when it isn't really necessary.  Second, I adjusted
> things so that we print a warning if the visibility map bit is set and
> the page-level bit is clear, since that should never happen in 9.2+.
> It is similar to the existing warning which catches the case where a
> page is marked all-visible despite containing dead tuples.
>
> Comments?
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
> http://archives.postgresql.org/pgsql-hackers/2012-04/msg00682.php
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>


-- 
GaoZengqi
pgf...@gmail.com
zengqi...@gmail.com

Re: [HACKERS] pg_rewind failure by file deletion in source server

[Michael Paquier]

On Sun, Aug 2, 2015 at 4:01 AM, Heikki Linnakangas wrote:
> Perhaps it's best if we copy all the WAL files from source in copy-mode, but
> not in libpq mode. Regarding old WAL files in the target, it's probably best
> to always leave them alone. They should do no harm, and as a general
> principle it's best to avoid destroying evidence.
>
> It'd be nice to get some fix for this for alpha2, so I'll commit a fix to do
> that on Monday, unless we come to a different conclusion before that.

+1. Both things sound like a good plan to me.
-- 
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] nodes/*funcs.c inconsistencies

[Noah Misch]

On Sun, Aug 02, 2015 at 11:31:16PM -0400, Tom Lane wrote:
> Stephen Frost  writes:
> > Noah,
> >> A fresh audit found the attached problems new in 9.5[1].  Most are cosmetic
> >> INT/UINT or field order corrections.  The non-cosmetic changes involve
> >> CustomPath, CustomScan, and CreatePolicyStmt.  Feature committers, if the
> >> existing treatments (ignore custom_plans/custom_paths fields; copy/compare
> >> "cmd" string pointer as a scalar) were deliberate, please let me know.
> 
> > Thanks for the review.  The change you have is correct for
> > CreatePolicyStmt, at least.  I imagine I confused it with polcmd, which
> > is actually just a char.
> 
> > Barring objections, I'll change it to cmd_name after your commit, to
> > reduce the chances of future confusion.

The existing identifier seems fine, but won't I mind that change, either.

> Both of you please keep in mind that these "cosmetic" changes are
> initdb-forcing, at least if they affect node types that can appear
> in stored rules.

Right; Stephen's does not force initdb, but some of what I posted does so.

> That being the case, it would probably be a good idea to get them done
> before alpha2, as there may not be a good opportunity afterwards.

Freedom to bump catversion after alpha2 will be barely-distinguishable from
freedom to do so now.  I have planned to leave my usual comment period of a
few days, though skipping that would be rather innocuous in this case.


-- 
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] nodes/*funcs.c inconsistencies

[Tom Lane]

Stephen Frost  writes:
> Noah,
>> A fresh audit found the attached problems new in 9.5[1].  Most are cosmetic
>> INT/UINT or field order corrections.  The non-cosmetic changes involve
>> CustomPath, CustomScan, and CreatePolicyStmt.  Feature committers, if the
>> existing treatments (ignore custom_plans/custom_paths fields; copy/compare
>> "cmd" string pointer as a scalar) were deliberate, please let me know.

> Thanks for the review.  The change you have is correct for
> CreatePolicyStmt, at least.  I imagine I confused it with polcmd, which
> is actually just a char.

> Barring objections, I'll change it to cmd_name after your commit, to
> reduce the chances of future confusion.

Both of you please keep in mind that these "cosmetic" changes are
initdb-forcing, at least if they affect node types that can appear
in stored rules.

That being the case, it would probably be a good idea to get them done
before alpha2, as there may not be a good opportunity afterwards.

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] nodes/*funcs.c inconsistencies

[Noah Misch]

On Mon, Aug 03, 2015 at 01:32:10AM +, Kouhei Kaigai wrote:
> > On Mon, Apr 16, 2012 at 06:25:15AM -0400, Noah Misch wrote:
> > > I observed these inconsistencies in node support functions:
> > 
> > A fresh audit found the attached problems new in 9.5[1].  Most are cosmetic
> > INT/UINT or field order corrections.  The non-cosmetic changes involve
> > CustomPath, CustomScan, and CreatePolicyStmt.  Feature committers, if the
> > existing treatments (ignore custom_plans/custom_paths fields; copy/compare
> > "cmd" string pointer as a scalar) were deliberate, please let me know.
> >
> Thanks for your works.
> 
> I also noticed one other inconsistent point; _outMergeJoin() dumps
> mergeNullsFirst[] array but it does not use booltostr() macro.

Good catch.  All supported branches work that way, and it's not wrong.  I
recommend keeping it unchanged.


-- 
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] Improving test coverage of extensions with pg_dump

[Michael Paquier]

On Thu, Jul 30, 2015 at 5:35 PM, Michael Paquier
 wrote:
> Note as well that I will be fine with any decision taken by the
> committer who picks up this, this test case is useful by itself in any
> case.

And I just recalled that I actually did no tests for this thing on
Windows. As this uses the TAP facility, I think that it makes most
sense to run it with tapcheck instead of modulescheck in vcregress.pl
because of its dependency with IPC::Run. The compilation with MSVC is
fixed as well.
-- 
Michael
From a56e2ed296533e30bff47df11f68f0f415ae8a3f Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Sun, 2 Aug 2015 19:11:58 -0700
Subject: [PATCH] Add TAP test for pg_dump checking data dump of extension 
 tables

The test added checks the data dump ordering of tables added in an extension
linked among them with foreign keys. In order to do that, a test extension in
the set of TAP tests of pg_dump, combined with a TAP test script making use
of it.
---
 src/test/modules/Makefile |  1 +
 src/test/modules/tables_fk/.gitignore |  1 +
 src/test/modules/tables_fk/Makefile   | 24 +
 src/test/modules/tables_fk/README |  5 
 src/test/modules/tables_fk/t/001_dump_test.pl | 39 +++
 src/test/modules/tables_fk/tables_fk--1.0.sql | 20 ++
 src/test/modules/tables_fk/tables_fk.control  |  5 
 src/tools/msvc/Mkvcbuild.pm   |  2 +-
 src/tools/msvc/vcregress.pl   |  3 +++
 9 files changed, 99 insertions(+), 1 deletion(-)
 create mode 100644 src/test/modules/tables_fk/.gitignore
 create mode 100644 src/test/modules/tables_fk/Makefile
 create mode 100644 src/test/modules/tables_fk/README
 create mode 100644 src/test/modules/tables_fk/t/001_dump_test.pl
 create mode 100644 src/test/modules/tables_fk/tables_fk--1.0.sql
 create mode 100644 src/test/modules/tables_fk/tables_fk.control

diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 8213e23..683e9cb 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -7,6 +7,7 @@ include $(top_builddir)/src/Makefile.global
 SUBDIRS = \
 		  commit_ts \
 		  dummy_seclabel \
+		  tables_fk \
 		  test_ddl_deparse \
 		  test_parser \
 		  test_rls_hooks \
diff --git a/src/test/modules/tables_fk/.gitignore b/src/test/modules/tables_fk/.gitignore
new file mode 100644
index 000..b6a2a01
--- /dev/null
+++ b/src/test/modules/tables_fk/.gitignore
@@ -0,0 +1 @@
+/tmp_check/
diff --git a/src/test/modules/tables_fk/Makefile b/src/test/modules/tables_fk/Makefile
new file mode 100644
index 000..d018517
--- /dev/null
+++ b/src/test/modules/tables_fk/Makefile
@@ -0,0 +1,24 @@
+# src/test/modules/tables_fk/Makefile
+
+EXTENSION = tables_fk
+DATA = tables_fk--1.0.sql
+PGFILEDESC = "tables_fk - set of dumpable tables linked by foreign keys"
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/tables_fk
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
+
+check: all
+	$(prove_check)
+
+installcheck: install
+	$(prove_installcheck)
+
+temp-install: EXTRA_INSTALL=src/test/modules/tables_fk
diff --git a/src/test/modules/tables_fk/README b/src/test/modules/tables_fk/README
new file mode 100644
index 000..049b75c
--- /dev/null
+++ b/src/test/modules/tables_fk/README
@@ -0,0 +1,5 @@
+tables_fk
+=
+
+Simple module used to check data dump ordering of pg_dump with tables
+linked with foreign keys using TAP tests.
diff --git a/src/test/modules/tables_fk/t/001_dump_test.pl b/src/test/modules/tables_fk/t/001_dump_test.pl
new file mode 100644
index 000..9d3d5ff
--- /dev/null
+++ b/src/test/modules/tables_fk/t/001_dump_test.pl
@@ -0,0 +1,39 @@
+use strict;
+use warnings;
+use TestLib;
+use Test::More tests => 4;
+
+my $tempdir = tempdir;
+
+start_test_server $tempdir;
+
+psql 'postgres', 'CREATE EXTENSION tables_fk';
+
+# Insert some data before running the dump, this is needed to check
+# consistent data dump of tables with foreign key dependencies
+psql 'postgres', 'INSERT INTO cc_tab_fkey VALUES (1)';
+psql 'postgres', 'INSERT INTO bb_tab_fkey VALUES (1)';
+psql 'postgres', 'INSERT INTO aa_tab_fkey VALUES (1)';
+
+# Create a table depending on a FK defined in the extension
+psql 'postgres', 'CREATE TABLE dd_tab_fkey (id int REFERENCES bb_tab_fkey(id))';
+psql 'postgres', 'INSERT INTO dd_tab_fkey VALUES (1)';
+
+# Take a dump then re-deploy it to a new database
+command_ok(['pg_dump', '-d', 'postgres', '-f', "$tempdir/dump.sql"],
+		   'taken dump with installed extension');
+command_ok(['createdb', 'foobar1'], 'created new database to redeploy dump');
+command_ok(['psql', '--set=ON_ERROR_STOP=on', '-d', 'foobar1', '-f',
+			"$tempdir/dump.sql"], 'dump restored');
+
+# And check its content
+my $result = psql 'foobar1',
+	q{SELECT id FROM aa_tab_fkey UNION ALL
+SELECT id

Re: [HACKERS] Explanation for intermittent buildfarm pg_upgradecheck failures

[Michael Paquier]

On Mon, Aug 3, 2015 at 1:30 AM, Tom Lane  wrote:
> I haven't looked to find out why the unlinks happen in this order, but on
> a heavily loaded machine, it's certainly possible that the process would
> lose the CPU after unlink("postmaster.pid"), and then a new postmaster
> could get far enough to see the socket lock file still there.  So that
> would account for low-probability failures in the pg_upgradecheck test,
> which is exactly what we've been seeing.

Oh... This may explain the different failures seen with TAP tests on
hamster, and axolotl with pg_upgrade as well. It is rather easy to get
them heavily loaded.
-- 
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] nodes/*funcs.c inconsistencies

[Kouhei Kaigai]

> On Mon, Apr 16, 2012 at 06:25:15AM -0400, Noah Misch wrote:
> > I observed these inconsistencies in node support functions:
> 
> A fresh audit found the attached problems new in 9.5[1].  Most are cosmetic
> INT/UINT or field order corrections.  The non-cosmetic changes involve
> CustomPath, CustomScan, and CreatePolicyStmt.  Feature committers, if the
> existing treatments (ignore custom_plans/custom_paths fields; copy/compare
> "cmd" string pointer as a scalar) were deliberate, please let me know.
>
Thanks for your works.

I also noticed one other inconsistent point; _outMergeJoin() dumps
mergeNullsFirst[] array but it does not use booltostr() macro.

Best regards,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 



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

Re: [HACKERS] patch : Allow toast tables to be moved to a different tablespace

[Andreas Karlsson]

On 07/15/2015 09:30 PM, Robert Haas wrote:

On Tue, Jul 14, 2015 at 5:57 PM, Jim Nasby  wrote:

On 7/7/15 7:07 AM, Andres Freund wrote:

On 2015-07-03 18:03:58 -0400, Tom Lane wrote:

I have just looked through this thread, and TBH I think we should reject
this patch altogether --- not RWF, but "no we don't want this".  The
use-case remains hypothetical: no performance numbers showing a
real-world
benefit have been exhibited AFAICS.



It's not that hard to imagine a performance benefit though? If the
toasted column is accessed infrequently/just after filtering on other
columns (not uncommon) it could very well be beneficial to put the main
table on fast storage (SSD) and the toast table on slow storage
(spinning rust).

I've seen humoungous toast tables that are barely ever read for tables
that are constantly a couple times in the field.


+1. I know of one case where the heap was ~8GB and TOAST was over 400GB.


Yeah, I think that's a good argument for this.  I have to admit that
I'm a bit fatigued by this thread - it started out as a "learn
PostgreSQL" project, and we iterated through a few design problems
that made me kind of worried about what sort of state the patch was in
- and now this patch is more than 4 years old.  But if some committer
still has the energy to go through it in detail and make sure that all
of the problems have been fixed and that the patch is, as Andreas
says, in good shape, then I don't see why we shouldn't take it.


I personally think the patch is in a decent shape, and a worthwhile 
feature. I agree though with Tom's objections about the pg_dump code.


I do not have enough time or interest right now to fix up this patch 
(this is not a feature I personally have a lot of interest in), but if 
someone else wishes to I do not think it would be too much work.


Andreas



--
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] nodes/*funcs.c inconsistencies

[Stephen Frost]

Noah,

* Noah Misch (n...@leadboat.com) wrote:
> On Mon, Apr 16, 2012 at 06:25:15AM -0400, Noah Misch wrote:
> > I observed these inconsistencies in node support functions:
> 
> A fresh audit found the attached problems new in 9.5[1].  Most are cosmetic
> INT/UINT or field order corrections.  The non-cosmetic changes involve
> CustomPath, CustomScan, and CreatePolicyStmt.  Feature committers, if the
> existing treatments (ignore custom_plans/custom_paths fields; copy/compare
> "cmd" string pointer as a scalar) were deliberate, please let me know.

Thanks for the review.  The change you have is correct for
CreatePolicyStmt, at least.  I imagine I confused it with polcmd, which
is actually just a char.

Barring objections, I'll change it to cmd_name after your commit, to
reduce the chances of future confusion.

Thanks again!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] upgrade failure from 9.5 to head

[Stephen Frost]

* Andres Freund (and...@anarazel.de) wrote:
> On 2015-08-01 19:13:05 -0400, Noah Misch wrote:
> > On Wed, Jul 29, 2015 at 04:42:55PM -0400, Andrew Dunstan wrote:
> > > The next hump is this, in restoring contrib_regression_test_ddl_parse:
> > > 
> > >pg_restore: creating FUNCTION "public"."text_w_default_in("cstring")"
> > >pg_restore: [archiver (db)] Error while PROCESSING TOC:
> > >pg_restore: [archiver (db)] Error from TOC entry 243; 1255 62534
> > >FUNCTION text_w_default_in("cstring") buildfarm
> > >pg_restore: [archiver (db)] could not execute query: ERROR: pg_type
> > >OID value not set when in binary upgrade mode
> > > Command was: CREATE FUNCTION "text_w_default_in"("cstring")
> > >RETURNS "text_w_default"
> > > LANGUAGE "internal" STABLE STRICT
> > > AS $$texti...
> > > 
> > > Is this worth bothering about, or should I simply remove the database 
> > > before
> > > trying to upgrade?
> > 
> > That's a bug.  The test_ddl_deparse suite leaves a shell type, which
> > pg_upgrade fails to reproduce.  Whether to have pg_upgrade support that or
> > just error out cleanly is another question.
> 
> There seems little justification to not support shell types. We should
> also add a shell type to the standard regression testing database,
> they're "weird" enough that some increased exposure seems like a good
> idea.

+1.

I was doing testing the other day and ran into the "pg_dump doesn't
support shell types" issue and it was annoyingly confusing.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] upgrade failure from 9.5 to head

[Andres Freund]

On 2015-08-02 19:06:49 -0400, Andrew Dunstan wrote:
> I'm fine with fixing it, but what's the actual use case for a long lived
> shell type?

The use-case imo is that we shouldn't just break if somebody did
something stupid or was busy creating a new type while a scheduled
backup ran.

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] upgrade failure from 9.5 to head

[Andrew Dunstan]

On 08/02/2015 04:00 PM, Tom Lane wrote:

Andres Freund  writes:

On 2015-08-01 19:13:05 -0400, Noah Misch wrote:

That's a bug.  The test_ddl_deparse suite leaves a shell type, which
pg_upgrade fails to reproduce.  Whether to have pg_upgrade support that or
just error out cleanly is another question.

There seems little justification to not support shell types. We should
also add a shell type to the standard regression testing database,
they're "weird" enough that some increased exposure seems like a good
idea.

Agreed.  I was a bit surprised to find that pg_dump skips shell types,
actually.  Probably that's a hangover from when "create function foo()
returns bogus" would autocreate a shell type named "bogus".  In all
modern releases, it's fairly hard to accidentally create a shell type,
so we should probably assume that the user meant it to be there.





I'm fine with fixing it, but what's the actual use case for a long lived 
shell type?


cheers

andrew



--
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] nodes/*funcs.c inconsistencies

[Peter Geoghegan]

On Sun, Aug 2, 2015 at 3:07 PM, Peter Geoghegan  wrote:
> I'm surprised that this stuff was only ever used for logical decoding
> infrastructure so far.

On second thought, having tried it, one reason is that that breaks
things that are considered legitimate for historical reasons. For
example, AttrNumber is often used with READ_INT_FIELD(), which is an
int16. Whether or not it's worth fixing that by introducing a
READ_ATTRNUM_FIELD() (and so on) is not obvious to me.

-- 
Peter Geoghegan


-- 
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] nodes/*funcs.c inconsistencies

[Peter Geoghegan]

On Sun, Aug 2, 2015 at 1:28 PM, Noah Misch  wrote:
> A fresh audit found the attached problems new in 9.5[1].  Most are cosmetic
> INT/UINT or field order corrections.

I was responsible for a couple of the cosmetic ones. Sorry about that.

It occurs to me that we could do a little more to prevent this
automatically. Couldn't we adopt
AssertVariableIsOfType()/AssertVariableIsOfTypeMacro() to macros like
READ_UINT_FIELD()?

I'm surprised that this stuff was only ever used for logical decoding
infrastructure so far.
-- 
Peter Geoghegan


-- 
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] nodes/*funcs.c inconsistencies

[Tom Lane]

Noah Misch  writes:
> On Mon, Apr 16, 2012 at 06:25:15AM -0400, Noah Misch wrote:
>> I observed these inconsistencies in node support functions:

> A fresh audit found the attached problems new in 9.5[1].

Many thanks for doing that; I'd had the same checking on my personal to-do
list, but now will not need to.

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] nodes/*funcs.c inconsistencies

[Noah Misch]

On Mon, Apr 16, 2012 at 06:25:15AM -0400, Noah Misch wrote:
> I observed these inconsistencies in node support functions:

A fresh audit found the attached problems new in 9.5[1].  Most are cosmetic
INT/UINT or field order corrections.  The non-cosmetic changes involve
CustomPath, CustomScan, and CreatePolicyStmt.  Feature committers, if the
existing treatments (ignore custom_plans/custom_paths fields; copy/compare
"cmd" string pointer as a scalar) were deliberate, please let me know.

Thanks,
nm

[1] The _equalCreateEventTrigStmt() cosmetic change is relevant as far back as
9.3, but I won't back-patch it further than the others (9.5).
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 7248440..1c8425d 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -647,6 +647,7 @@ _copyCustomScan(const CustomScan *from)
 * copy remainder of node
 */
COPY_SCALAR_FIELD(flags);
+   COPY_NODE_FIELD(custom_plans);
COPY_NODE_FIELD(custom_exprs);
COPY_NODE_FIELD(custom_private);
COPY_NODE_FIELD(custom_scan_tlist);
@@ -1925,9 +1926,9 @@ _copyOnConflictExpr(const OnConflictExpr *from)
COPY_SCALAR_FIELD(action);
COPY_NODE_FIELD(arbiterElems);
COPY_NODE_FIELD(arbiterWhere);
+   COPY_SCALAR_FIELD(constraint);
COPY_NODE_FIELD(onConflictSet);
COPY_NODE_FIELD(onConflictWhere);
-   COPY_SCALAR_FIELD(constraint);
COPY_SCALAR_FIELD(exclRelIndex);
COPY_NODE_FIELD(exclRelTlist);
 
@@ -4082,7 +4083,7 @@ _copyCreatePolicyStmt(const CreatePolicyStmt *from)
 
COPY_STRING_FIELD(policy_name);
COPY_NODE_FIELD(table);
-   COPY_SCALAR_FIELD(cmd);
+   COPY_STRING_FIELD(cmd);
COPY_NODE_FIELD(roles);
COPY_NODE_FIELD(qual);
COPY_NODE_FIELD(with_check);
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 6597dbc..1d6c43c 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -759,9 +759,9 @@ _equalOnConflictExpr(const OnConflictExpr *a, const 
OnConflictExpr *b)
COMPARE_SCALAR_FIELD(action);
COMPARE_NODE_FIELD(arbiterElems);
COMPARE_NODE_FIELD(arbiterWhere);
+   COMPARE_SCALAR_FIELD(constraint);
COMPARE_NODE_FIELD(onConflictSet);
COMPARE_NODE_FIELD(onConflictWhere);
-   COMPARE_SCALAR_FIELD(constraint);
COMPARE_SCALAR_FIELD(exclRelIndex);
COMPARE_NODE_FIELD(exclRelTlist);
 
@@ -1868,8 +1868,8 @@ _equalCreateEventTrigStmt(const CreateEventTrigStmt *a, 
const CreateEventTrigStm
 {
COMPARE_STRING_FIELD(trigname);
COMPARE_STRING_FIELD(eventname);
-   COMPARE_NODE_FIELD(funcname);
COMPARE_NODE_FIELD(whenclause);
+   COMPARE_NODE_FIELD(funcname);
 
return true;
 }
@@ -2074,7 +2074,7 @@ _equalCreatePolicyStmt(const CreatePolicyStmt *a, const 
CreatePolicyStmt *b)
 {
COMPARE_STRING_FIELD(policy_name);
COMPARE_NODE_FIELD(table);
-   COMPARE_SCALAR_FIELD(cmd);
+   COMPARE_STRING_FIELD(cmd);
COMPARE_NODE_FIELD(roles);
COMPARE_NODE_FIELD(qual);
COMPARE_NODE_FIELD(with_check);
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 86f3214..068724e 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -341,7 +341,7 @@ _outModifyTable(StringInfo str, const ModifyTable *node)
WRITE_NODE_FIELD(arbiterIndexes);
WRITE_NODE_FIELD(onConflictSet);
WRITE_NODE_FIELD(onConflictWhere);
-   WRITE_INT_FIELD(exclRelRTI);
+   WRITE_UINT_FIELD(exclRelRTI);
WRITE_NODE_FIELD(exclRelTlist);
 }
 
@@ -591,6 +591,7 @@ _outCustomScan(StringInfo str, const CustomScan *node)
_outScanInfo(str, (const Scan *) node);
 
WRITE_UINT_FIELD(flags);
+   WRITE_NODE_FIELD(custom_plans);
WRITE_NODE_FIELD(custom_exprs);
WRITE_NODE_FIELD(custom_private);
WRITE_NODE_FIELD(custom_scan_tlist);
@@ -1016,7 +1017,7 @@ _outGroupingFunc(StringInfo str, const GroupingFunc *node)
WRITE_NODE_FIELD(args);
WRITE_NODE_FIELD(refs);
WRITE_NODE_FIELD(cols);
-   WRITE_INT_FIELD(agglevelsup);
+   WRITE_UINT_FIELD(agglevelsup);
WRITE_LOCATION_FIELD(location);
 }
 
@@ -1532,9 +1533,9 @@ _outOnConflictExpr(StringInfo str, const OnConflictExpr 
*node)
WRITE_ENUM_FIELD(action, OnConflictAction);
WRITE_NODE_FIELD(arbiterElems);
WRITE_NODE_FIELD(arbiterWhere);
+   WRITE_OID_FIELD(constraint);
WRITE_NODE_FIELD(onConflictSet);
WRITE_NODE_FIELD(onConflictWhere);
-   WRITE_OID_FIELD(constraint);
WRITE_INT_FIELD(exclRelIndex);
WRITE_NODE_FIELD(exclRelTlist);
 }
@@ -1674,6 +1675,7 @@ _outCustomPath(StringInfo str, const CustomPath *node)
_outPathInfo(str, (const Path *) node);
 
WRITE_UINT_FIELD(flags);
+   WRITE_NODE_FIELD(custom_paths);
WRITE_N

Re: [HACKERS] [sqlsmith] Failed assertion in joinrels.c

[Tom Lane]

Piotr Stefaniak  writes:
> On 08/01/2015 05:59 PM, Tom Lane wrote:
>> Well, I certainly think all of these represent bugs:
>>> 1 | ERROR:  could not find pathkey item to sort

> sqlsmith was able to find these two queries that trigger the error on my 
> machine:

Hmm ... I see no error with these queries as of today's HEAD or
back-branch tips.  I surmise that this was triggered by one of the other
recently-fixed bugs, though the connection isn't obvious offhand.

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] upgrade failure from 9.5 to head

[Tom Lane]

Andres Freund  writes:
> On 2015-08-01 19:13:05 -0400, Noah Misch wrote:
>> That's a bug.  The test_ddl_deparse suite leaves a shell type, which
>> pg_upgrade fails to reproduce.  Whether to have pg_upgrade support that or
>> just error out cleanly is another question.

> There seems little justification to not support shell types. We should
> also add a shell type to the standard regression testing database,
> they're "weird" enough that some increased exposure seems like a good
> idea.

Agreed.  I was a bit surprised to find that pg_dump skips shell types,
actually.  Probably that's a hangover from when "create function foo()
returns bogus" would autocreate a shell type named "bogus".  In all
modern releases, it's fairly hard to accidentally create a shell type,
so we should probably assume that the user meant it to be there.

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] Null pointer passed as source to memcpy() in numeric.c:make_result() and numeric:set_var_from_var()

[Tom Lane]

Piotr Stefaniak  writes:
> these two queries will make the assertions below fail:
> SELECT STDDEV(0.0);
> SELECT 0.0 * 0;

Fixed, thanks.

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] Explanation for intermittent buildfarm pg_upgradecheck failures

[Tom Lane]

I wrote:
> Further experimentation says that 9.0-9.2 do this in the expected order;
> so somebody broke it during 9.3.

Depressingly enough, the somebody was me.  Fixed now.

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] MultiXact member wraparound protections are now enabled

[Peter Eisentraut]

On 7/28/15 9:20 AM, Robert Haas wrote:
> Well, I think that we can eventually downgrade or remove the message
> once (1) we've actually fixed all of the known multixact bugs and (2)
> a couple of years have gone by and most people are in the clear.

Fair enough.  But we should document this better in the future.


-- 
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] Explanation for intermittent buildfarm pg_upgradecheck failures

[Tom Lane]

I wrote:
> unlink("/tmp/.s.PGSQL.5432")= 0
> unlink("postmaster.pid")= 0
> unlink("/tmp/.s.PGSQL.5432.lock")   = 0
> exit_group(0)   = ?
> +++ exited with 0 +++

> I haven't looked to find out why the unlinks happen in this order, but on
> a heavily loaded machine, it's certainly possible that the process would
> lose the CPU after unlink("postmaster.pid"), and then a new postmaster
> could get far enough to see the socket lock file still there.  So that
> would account for low-probability failures in the pg_upgradecheck test,
> which is exactly what we've been seeing.

Further experimentation says that 9.0-9.2 do this in the expected order;
so somebody broke it during 9.3.

The lack of a close() on the postmaster socket goes all the way back
though.

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] No more libedit?! - openssl plans to switch to APL2

[Andres Freund]

On 2015-08-02 12:34:43 -0400, Robert Haas wrote:
> On Sat, Aug 1, 2015 at 11:14 AM, Andres Freund  wrote:
> > According to https://www.openssl.org/blog/blog/2015/08/01/cla/ openssl
> > is planning to relicense to the apache license 2.0. While APL2 is not
> > compatible with GLP2 it *is* compatible with GPL3.
> 
> What's the connection to libedit?

Some platforms have to use libedit because linking to both libreadline
and openssl is of debated legality. GPL prohibits additional
restrictions and openssl's license has an advertising clause which is
often interpreted violating the additional-restrictions clause.

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] LWLock deadlock and gdb advice

[Andres Freund]

On 2015-08-02 12:33:06 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > I plan to commit the patch tomorrow, so it's included in alpha2.
> 
> Please try to commit anything you want in alpha2 *today*.  I'd
> prefer to see some successful buildfarm cycles on such patches
> before we wrap.

Ok, will do that.


-- 
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] No more libedit?! - openssl plans to switch to APL2

[Robert Haas]

On Sat, Aug 1, 2015 at 11:14 AM, Andres Freund  wrote:
> According to https://www.openssl.org/blog/blog/2015/08/01/cla/ openssl
> is planning to relicense to the apache license 2.0. While APL2 is not
> compatible with GLP2 it *is* compatible with GPL3.

What's the connection to libedit?

-- 
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] LWLock deadlock and gdb advice

[Tom Lane]

Andres Freund  writes:
> I plan to commit the patch tomorrow, so it's included in alpha2.

Please try to commit anything you want in alpha2 *today*.  I'd
prefer to see some successful buildfarm cycles on such patches
before we wrap.

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

[HACKERS] Explanation for intermittent buildfarm pg_upgradecheck failures

[Tom Lane]

Observe the smoking gun at
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mule&dt=2015-08-02%2003%3A30%3A02

to wit this extract from pg_upgrade_server.log:

command: 
"/home/pg/build-farm-4.15.1/build/HEAD/pgsql.build/src/bin/pg_upgrade/tmp_check/install//home/pg/build-farm-4.15.1/build/HEAD/inst/bin/pg_ctl"
 -w -D 
"/home/pg/build-farm-4.15.1/build/HEAD/pgsql.build/src/bin/pg_upgrade/tmp_check/data.old"
 -o ""  stop >> "pg_upgrade_server.log" 2>&1
LOG:  received fast shutdown request
LOG:  aborting any active transactions
LOG:  shutting down
waiting for server to shut down.LOG:  database system is shut down
 done
server stopped

command: 
"/home/pg/build-farm-4.15.1/build/HEAD/pgsql.build/src/bin/pg_upgrade/tmp_check/install//home/pg/build-farm-4.15.1/build/HEAD/inst/bin/pg_ctl"
 -w -l "pg_upgrade_server.log" -D 
"/home/pg/build-farm-4.15.1/build/HEAD/pgsql.build/src/bin/pg_upgrade/tmp_check/data"
 -o "-p 57832 -b -c synchronous_commit=off -c fsync=off -c full_page_writes=off 
 -c listen_addresses='' -c unix_socket_permissions=0700 -c 
unix_socket_directories='/home/pg/build-farm-4.15.1/build/HEAD/pgsql.build/src/bin/pg_upgrade'"
 start >> "pg_upgrade_server.log" 2>&1
waiting for server to startFATAL:  lock file 
"/home/pg/build-farm-4.15.1/build/HEAD/pgsql.build/src/bin/pg_upgrade/.s.PGSQL.57832.lock"
 already exists
HINT:  Is another postmaster (PID 12295) using socket file 
"/home/pg/build-farm-4.15.1/build/HEAD/pgsql.build/src/bin/pg_upgrade/.s.PGSQL.57832"?
 stopped waiting
pg_ctl: could not start server
Examine the log output.

Now, the old postmaster clearly was shut down, so how come the socket lock
file still existed?  The answer is that what pg_ctl is watching for is for
the postmaster.pid file to disappear, and *the postmaster deletes its lock
files in the wrong order*.  strace'ing HEAD on my own machine shows this
sequence of kernel calls during postmaster stop:

wait4(-1, 0x723f3cbc, WNOHANG, NULL) = -1 ECHILD (No child processes)
stat("backup_label", 0x723f3bd0)= -1 ENOENT (No such file or directory)
munmap(0x7fd16e8f8000, 2316)= 0
unlink("/dev/shm/PostgreSQL.1804289383") = 0
semctl(1547862018, 0, IPC_RMID, 0)  = 0
semctl(1547894787, 0, IPC_RMID, 0)  = 0
semctl(1547927556, 0, IPC_RMID, 0)  = 0
semctl(1547960325, 0, IPC_RMID, 0)  = 0
semctl(1547993094, 0, IPC_RMID, 0)  = 0
semctl(1548025863, 0, IPC_RMID, 0)  = 0
semctl(1548058632, 0, IPC_RMID, 0)  = 0
semctl(1548091401, 0, IPC_RMID, 0)  = 0
shmdt(0x7fd16e8f9000)   = 0
munmap(0x7fd165b3c000, 148488192)   = 0
shmctl(194314244, IPC_RMID, 0)  = 0
unlink("/tmp/.s.PGSQL.5432")= 0
unlink("postmaster.pid")= 0
unlink("/tmp/.s.PGSQL.5432.lock")   = 0
exit_group(0)   = ?
+++ exited with 0 +++

I haven't looked to find out why the unlinks happen in this order, but on
a heavily loaded machine, it's certainly possible that the process would
lose the CPU after unlink("postmaster.pid"), and then a new postmaster
could get far enough to see the socket lock file still there.  So that
would account for low-probability failures in the pg_upgradecheck test,
which is exactly what we've been seeing.

It seems clear to me that what the exit sequence ought to be is
unlink socket, unlink socket lock file, repeat for any additional
sockets, then unlink postmaster.pid.  Can anyone think of a reason
why the current ordering isn't a bug?

Another point that's rather obvious from the trace is that at no time
do we bother to close the postmaster's TCP socket; it only goes away when
the postmaster process dies.  So there's a second race condition here:
a new postmaster could be unable to bind() to the desired TCP port
because the old one still has it open.  I think we've seen unexplained
failures of the TCP-socket-in-use variety, too.

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

[HACKERS] pageinspect patch, for showing tuple data

[Nikolay Shaplov]

Hi!

I've created a patch for pageinspect that allows to see data stored in the 
tuple.

This patch has two main purposes:

1. Practical: Make manual DB recovery more simple
2. Educational: Seeing what data is actually stored in tuple, allows to get 
better understanding of how does postgres actually works.

This patch adds several new functions, available from SQL queries. All these 
functions are based on heap_page_items, but accept slightly different 
arguments and has one additional column at the result set:

heap_page_tuples - accepts relation name, and bulkno, and returns usual 
heap_page_items set with additional column that contain snapshot of tuple data 
area stored in bytea.

heap_page_tuples_attributes - same as heap_page_tuples, but instead of single 
tuple data bytea snapshot, it has array of bytea values, that were splitted 
into attributes as they would be spitted by nocachegetattr function (I 
actually reimplemented this function main algorithm to get this done)

heap_page_tuples_attrs_detoasted - same as heap_page_tuples_attrs, but all 
varlen attributes values that were compressed or TOASTed, are replaced with 
unTOASTed and uncompressed values.


There is also one strange function: _heap_page_items it is useless for 
practical purposes. As heap_page_items it accepts page data as bytea, but also 
get a relation name. It tries to apply tuple descriptor of that relation to 
that page data. 
This would allow you to try to read page data from one table using tuple 
descriptor from anther. A strange idea, one should say. But this will allow 
you: a) See how wrong data can be interpreted (educational purpose).
b) I have plenty of sanity check while reading parsing that tuple, for this 
function I've changed error level from ERROR to WARNING. This function will 
allow to write proper tests that all these checks work as they were designed 
(I hope to write these tests sooner or later)

I've also added raw tuple data output to original heap_page_items function, 
thought I am not sure if it is good idea. I just can add it there so I did it. 
May be it would be better to change it back for better backward compatibility.

Attached patched is in "almost ready" state. It has some formatting issues. 
I'd like to hear HACKER's opinion before finishing it and sending to 
commitfest.

-- 
Nikolay Shaplov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Companydiff --git a/contrib/pageinspect/Makefile b/contrib/pageinspect/Makefile
index aec5258..e4bc1af 100644
--- a/contrib/pageinspect/Makefile
+++ b/contrib/pageinspect/Makefile
@@ -5,7 +5,7 @@ OBJS		= rawpage.o heapfuncs.o btreefuncs.o fsmfuncs.o \
 		  brinfuncs.o ginfuncs.o $(WIN32RES)
 
 EXTENSION = pageinspect
-DATA = pageinspect--1.3.sql pageinspect--1.2--1.3.sql \
+DATA = pageinspect--1.4.sql pageinspect--1.3--1.4.sql pageinspect--1.2--1.3.sql \
 	pageinspect--1.1--1.2.sql pageinspect--1.0--1.1.sql \
 	pageinspect--unpackaged--1.0.sql
 PGFILEDESC = "pageinspect - functions to inspect contents of database pages"
diff --git a/contrib/pageinspect/heapfuncs.c b/contrib/pageinspect/heapfuncs.c
index 8d1666c..e296619 100644
--- a/contrib/pageinspect/heapfuncs.c
+++ b/contrib/pageinspect/heapfuncs.c
@@ -29,7 +29,12 @@
 #include "funcapi.h"
 #include "utils/builtins.h"
 #include "miscadmin.h"
+#include "utils/array.h"
+#include "utils/rel.h"
+#include "catalog/namespace.h"
+#include "catalog/pg_type.h"
 
+#include "rawpage.h"
 
 /*
  * bits_to_text
@@ -53,6 +58,9 @@ bits_to_text(bits8 *bits, int len)
 	return str;
 }
 
+Datum heap_page_items_internal(FunctionCallInfo fcinfo, bytea *raw_page, text *relname, int error_level, bool do_detoast);
+void fill_header_data(FuncCallContext *fctx, Datum *values, bool *nulls, bool do_detoast);
+void split_tuple_data(FuncCallContext *fctx, HeapTupleHeader tuphdr, Datum *values, bool *nulls, bool do_detoast);
 
 /*
  * heap_page_items
@@ -66,38 +74,98 @@ typedef struct heap_page_items_state
 	TupleDesc	tupd;
 	Page		page;
 	uint16		offset;
+	TupleDesc	page_tuple_desc;
+	int		raw_page_size;
+	int		error_level;
 } heap_page_items_state;
 
 Datum
 heap_page_items(PG_FUNCTION_ARGS)
 {
 	bytea	   *raw_page = PG_GETARG_BYTEA_P(0);
+	text	   *relname = PG_NARGS()==2 ? PG_GETARG_TEXT_P(1) : NULL;
+
+	/*
+	 * Error level is only used while splitting tuple into array of attributes
+	 * This is done only for _heap_page_items function. _heap_page_items is intended
+	 * for educational and research purposes, so we should change all error checks
+	 * to warnings
+	 */
+	return heap_page_items_internal(fcinfo, raw_page, relname, WARNING, false);
+}
+
+PG_FUNCTION_INFO_V1(heap_page_tuples);
+Datum
+heap_page_tuples(PG_FUNCTION_ARGS)
+{
+	text   *relname = PG_GETARG_TEXT_P(0);
+	uint32 blkno= PG_GETARG_UINT32(1);
+	bytea *raw_page;
+
+	if (SRF_IS_FIRSTCALL())
+	{
+		raw_page = get_raw_page_internal(relname, MAIN_FORKNUM, blkno);
+	}
+	return heap_page_items_internal(fcinfo, raw_page, NULL, ERROR, false);
+}
+
+PG_FUN

Re: [HACKERS] LWLock deadlock and gdb advice

[Andres Freund]

Hi Jeff, Heikki,

On 2015-07-31 09:48:28 -0700, Jeff Janes wrote:
> I had run it for 24 hours, while it usually took less than 8 hours to look
> up before.  I did see it within a few minutes one time when I checked out a
> new HEAD and then forgot to re-apply your or Heikki's patch.
> 
> But now I've got the same situation again, after 15 hours, with your
> patch.  This is probably all down to luck.  The only differences that I can
> think of is that I advanced the base to e8e86fbc8b3619da54c, and turned on
> JJ_vac and set log_autovacuum_min_duration=0.

It's quite possible that you hit the remaining race-condition that
Heikki was talking about. So that'd make it actually likely to be hit
slightly later - but as you say this is just a game of chance.

I've attached a version of the patch that should address Heikki's
concern. It imo also improves the API and increases debuggability by not
having stale variable values in the variables anymore. (also attached is
a minor optimization that Heikki has noticed)

I plan to commit the patch tomorrow, so it's included in alpha2.

Regards,

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] LWLock deadlock and gdb advice

[Andres Freund]

On 2015-08-02 17:04:07 +0200, Andres Freund wrote:
> I've attached a version of the patch that should address Heikki's
> concern. It imo also improves the API and increases debuggability by not
> having stale variable values in the variables anymore. (also attached is
> a minor optimization that Heikki has noticed)

...
>From 17b4e0692ebd91a92448b0abb8cf4438a93a29d6 Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Fri, 31 Jul 2015 20:20:43 +0200
Subject: [PATCH 1/2] Fix issues around the "variable" support in the lwlock
 infrastructure.

The lwlock scalability work introduced two race conditions into the
lwlock variable support provided for xlog.c. First, and harmlessly on
most platforms, it set/read the variable without the spinlock in some
places. Secondly, due to the removal of the spinlock, it was possible
that a backend missed changes to the variable's state if it changed in
the wrong moment because checking the lock's state, the variable's state
and the queuing are not protected by a single spinlock acquisition
anymore.

To fix first move resetting the variable's from LWLockAcquireWithVar to
WALInsertLockRelease, via a new function LWLockReleaseClearVar. That
prevents issues around waiting for a variable's value to change when a
new locker has acquired the lock, but not yet set the value. Secondly
re-check that the variable hasn't changed after enqueing, that prevents
the issue that the lock has been released and already re-acquired by the
time the woken up backend checks for the lock's state.

Reported-By: Jeff Janes
Analyzed-By: Heikki Linnakangas
Reviewed-By: Heikki Linnakangas
Discussion: 5592db35.2060...@iki.fi
Backpatch: 9.5, where the lwlock scalability went in
---
 src/backend/access/transam/xlog.c |  34 ---
 src/backend/storage/lmgr/lwlock.c | 193 +-
 src/include/storage/lwlock.h  |   2 +-
 3 files changed, 129 insertions(+), 100 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 1dd31b3..939813e 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -1408,9 +1408,7 @@ WALInsertLockAcquire(void)
 	 * The insertingAt value is initially set to 0, as we don't know our
 	 * insert location yet.
 	 */
-	immed = LWLockAcquireWithVar(&WALInsertLocks[MyLockNo].l.lock,
- &WALInsertLocks[MyLockNo].l.insertingAt,
- 0);
+	immed = LWLockAcquire(&WALInsertLocks[MyLockNo].l.lock, LW_EXCLUSIVE);
 	if (!immed)
 	{
 		/*
@@ -1435,26 +1433,28 @@ WALInsertLockAcquireExclusive(void)
 	int			i;
 
 	/*
-	 * When holding all the locks, we only update the last lock's insertingAt
-	 * indicator.  The others are set to 0x, which is higher
-	 * than any real XLogRecPtr value, to make sure that no-one blocks waiting
-	 * on those.
+	 * When holding all the locks, all but the last lock's insertingAt
+	 * indicator is set to 0x, which is higher than any real
+	 * XLogRecPtr value, to make sure that no-one blocks waiting on those.
 	 */
 	for (i = 0; i < NUM_XLOGINSERT_LOCKS - 1; i++)
 	{
-		LWLockAcquireWithVar(&WALInsertLocks[i].l.lock,
-			 &WALInsertLocks[i].l.insertingAt,
-			 PG_UINT64_MAX);
+		LWLockAcquire(&WALInsertLocks[i].l.lock, LW_EXCLUSIVE);
+		LWLockUpdateVar(&WALInsertLocks[i].l.lock,
+		&WALInsertLocks[i].l.insertingAt,
+		PG_UINT64_MAX);
 	}
-	LWLockAcquireWithVar(&WALInsertLocks[i].l.lock,
-		 &WALInsertLocks[i].l.insertingAt,
-		 0);
+	/* Variable value reset to 0 at release */
+	LWLockAcquire(&WALInsertLocks[i].l.lock, LW_EXCLUSIVE);
 
 	holdingAllLocks = true;
 }
 
 /*
  * Release our insertion lock (or locks, if we're holding them all).
+ *
+ * NB: Reset all variables to 0, so they cause LWLockWaitForVar to block the
+ * next time the lock is acquired.
  */
 static void
 WALInsertLockRelease(void)
@@ -1464,13 +1464,17 @@ WALInsertLockRelease(void)
 		int			i;
 
 		for (i = 0; i < NUM_XLOGINSERT_LOCKS; i++)
-			LWLockRelease(&WALInsertLocks[i].l.lock);
+			LWLockReleaseClearVar(&WALInsertLocks[i].l.lock,
+  &WALInsertLocks[i].l.insertingAt,
+  0);
 
 		holdingAllLocks = false;
 	}
 	else
 	{
-		LWLockRelease(&WALInsertLocks[MyLockNo].l.lock);
+		LWLockReleaseClearVar(&WALInsertLocks[MyLockNo].l.lock,
+			  &WALInsertLocks[MyLockNo].l.insertingAt,
+			  0);
 	}
 }
 
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index e5566d1..ae03eb1 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -10,13 +10,15 @@
  * locking should be done with the full lock manager --- which depends on
  * LWLocks to protect its shared state.
  *
- * In addition to exclusive and shared modes, lightweight locks can be used
- * to wait until a variable changes value.  The variable is initially set
- * when the lock is acquired with LWLockAcquireWithVar, and can be updated
+ * In addition to exclusive and shared modes, lightweight lo

Re: [HACKERS] upgrade failure from 9.5 to head

[Andres Freund]

On 2015-08-01 19:13:05 -0400, Noah Misch wrote:
> On Wed, Jul 29, 2015 at 04:42:55PM -0400, Andrew Dunstan wrote:
> > The next hump is this, in restoring contrib_regression_test_ddl_parse:
> > 
> >pg_restore: creating FUNCTION "public"."text_w_default_in("cstring")"
> >pg_restore: [archiver (db)] Error while PROCESSING TOC:
> >pg_restore: [archiver (db)] Error from TOC entry 243; 1255 62534
> >FUNCTION text_w_default_in("cstring") buildfarm
> >pg_restore: [archiver (db)] could not execute query: ERROR: pg_type
> >OID value not set when in binary upgrade mode
> > Command was: CREATE FUNCTION "text_w_default_in"("cstring")
> >RETURNS "text_w_default"
> > LANGUAGE "internal" STABLE STRICT
> > AS $$texti...
> > 
> > Is this worth bothering about, or should I simply remove the database before
> > trying to upgrade?
> 
> That's a bug.  The test_ddl_deparse suite leaves a shell type, which
> pg_upgrade fails to reproduce.  Whether to have pg_upgrade support that or
> just error out cleanly is another question.

There seems little justification to not support shell types. We should
also add a shell type to the standard regression testing database,
they're "weird" enough that some increased exposure seems like a good
idea.


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

[HACKERS] Minimum tuple threshold to decide last pass of VACUUM

[Michael Paquier]

Hi all,

Commit 4046e58c (dated of 2001) has introduced the following comment
in vacuumlazy.c:
+   /* If any tuples need to be deleted, perform final vacuum cycle */
+   /* XXX put a threshold on min nuber of tuples here? */
+   if (vacrelstats->num_dead_tuples > 0)
In short, we may want to have a reloption to decide if we do or not
the last pass of VACUUM or not depending on a given number of
remaining tuples. Is this still something we would like to have?

Regards,
-- 
Michael


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

[HACKERS] [sqlsmith] subplan variable reference / unassigned NestLoopParams (was: [sqlsmith] Failed assertion in joinrels.c)

[Andreas Seltenreich]

Tom Lane writes:

> Well, I certainly think all of these represent bugs:
>
>>  3 | ERROR:  plan should not reference subplan's variable
>>  2 | ERROR:  failed to assign all NestLoopParams to plan nodes

These appear to be related.  The following query produces the former,
but if you replace the very last reference of provider with the literal
'bar', it raises the latter error.

select 1 from
  pg_catalog.pg_shseclabel as rel_09
  inner join public.rtest_view2 as rel_32
  left join pg_catalog.pg_roles as rel_33
  on (rel_32.a = rel_33.rolconnlimit )
  on (rel_09.provider = rel_33.rolpassword )
left join pg_catalog.pg_user as rel_35
on (rel_33.rolconfig = rel_35.useconfig )
where ( ((rel_09.provider ~<~ 'foo')
  and (rel_35.usename ~* rel_09.provider)));

,[ FWIW: git bisect run ]
| first bad commit: [e83bb10d6dcf05a666d4ada00d9788c7974ad378]
| Adjust definition of cheapest_total_path to work better with LATERAL.
`

regards,
Andreas


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

[HACKERS] Incorrect comment about abbreviated keys

[Peter Geoghegan]

Attached patch fixes this issue. This was missed by
78efd5c1edb59017f06ef96773e64e6539bfbc86

-- 
Peter Geoghegan
From 47ba4759c3d460fa100f0c218b2b06834abfb3f5 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan 
Date: Sun, 2 Aug 2015 02:07:55 -0700
Subject: [PATCH] Fix comment.

Commit 78efd5c1edb59017f06ef96773e64e6539bfbc86 overlooked this.
---
 src/backend/utils/sort/tuplesort.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index 435041a..1e62a73 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -356,9 +356,9 @@ struct Tuplesortstate
 
 	/*
 	 * Additional state for managing "abbreviated key" sortsupport routines
-	 * (which currently may be used by all cases except the Datum sort case
-	 * and hash index case).  Tracks the intervals at which the optimization's
-	 * effectiveness is tested.
+	 * (which currently may be used by all cases except the hash index case).
+	 * Tracks the intervals at which the optimization's effectiveness is
+	 * tested.
 	 */
 	int64		abbrevNext;		/* Tuple # at which to next check
  * applicability */
-- 
1.9.1


-- 
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] [sqlsmith] Failed assertion in joinrels.c

[Peter Geoghegan]

On Fri, Jul 31, 2015 at 5:56 PM, Andreas Seltenreich  wrote:
> sqlsmith triggered the following assertion in master (c188204).

Thanks for writing sqlsmith. It seems like a great tool.

I wonder, are you just running the tool with assertions enabled when
PostgreSQL is built? If so, it might make sense to make various
problems more readily detected. As you may know, Clang has a pretty
decent option called AddressSanitizer that can detect memory errors as
they occur with an overhead that is not excessive. One might use the
following configure arguments when building PostgreSQL to use
AddressSanitizer:

./configure CC=clang CFLAGS='-O1 -g -fsanitize=address
-fno-omit-frame-pointer -fno-optimize-sibling-calls' --enable-cassert

Of course, it remains to be seen if this pays for itself. Apparently
the tool has about a 2x overhead [1]. I'm really not sure that you'll
find any more bugs this way, but it's certainly possible that you'll
find a lot more. Given your success in finding bugs without using
AddressSanitizer, introducing it may be premature.

[1] http://clang.llvm.org/docs/AddressSanitizer.html#introduction
-- 
Peter Geoghegan


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