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

[Thomas Munro]

On Tue, Feb 7, 2017 at 5:43 PM, Peter Geoghegan  wrote:
> However, there are some specific implementation issues with this that
> I didn't quite anticipate. I would like to get feedback on these
> issues now, from both Thomas and Robert. The issues relate to how much
> the patch can or should "buy into resource management". You might
> guess that this new resource management code is something that should
> live in fd.c, alongside the guts of temp file resource management,
> within the function FileClose(). That way, it would be called by every
> possible path that might delete a temp file, including
> ResourceOwnerReleaseInternal(). That's not what I've done, though.
> Instead, refcount management is limited to a few higher level routines
> in buffile.c. Initially, resource management in FileClose() is made to
> assume that it must delete the file. Then, if and when directed to by
> BufFileClose()/refcount, a backend may determine that it is not its
> job to do the deletion -- it will not be the one that must "turn out
> the lights", and so indicates to FileClose() that it should not delete
> the file after all (it should just release vFDs, close(), and so on).
> Otherwise, when refcount reaches zero, temp files are deleted by
> FileClose() in more or less the conventional manner.
>
> The fact that there could, in general, be any error that causes us to
> attempt a double-deletion (deletion of a temp file from more than one
> backend) for a time is less of a problem than you might think. This is
> because there is a risk of this only for as long as two backends hold
> open the file at the same time. In the case of parallel CREATE INDEX,
> this is now the shortest possible period of time, since workers close
> their files using BufFileClose() immediately after the leader wakes
> them up from a quiescent state. And, if that were to actually happen,
> say due to some random OOM error during that small window, the
> consequence is no worse than an annoying log message: "could not
> unlink file..." (this would come from the second backend that
> attempted an unlink()). You would not see this when a worker raised an
> error due to a duplicate violation, or any other routine problem, so
> it should really be almost impossible.
>
> That having been said, this probably *is* a problematic restriction in
> cases where a temp file's ownership is not immediately handed over
> without concurrent sharing. What happens to be a small window for the
> parallel CREATE INDEX patch probably wouldn't be a small window for
> parallel hash join.   :-(
>
> It's not hard to see why I would like to do things this way. Just look
> at ResourceOwnerReleaseInternal(). Any release of a file happens
> during RESOURCE_RELEASE_AFTER_LOCKS, whereas the release of dynamic
> shared memory segments happens earlier, during
> RESOURCE_RELEASE_BEFORE_LOCKS. ISTM that the only sensible way to
> implement a refcount is using dynamic shared memory, and that seems
> hard. There are additional reasons why I suggest we go this way, such
> as the fact that all the relevant state belongs to BufFile, which is
> implemented a layer above all of the guts of resource management of
> temp files within fd.c. I'd have to replicate almost all state in fd.c
> to make it all work, which seems like a big modularity violation.
>
> Does anyone have any suggestions on how to tackle this?

Hmm.  One approach might be like this:

1.  There is a shared refcount which is incremented when you open a
shared file and decremented if you optionally explicitly 'release' it.
(Not when you close it, because we can't allow code that may be run
during RESOURCE_RELEASE_AFTER_LOCKS to try to access the DSM segment
after it has been unmapped; more generally, creating destruction order
dependencies between different kinds of resource-manager-cleaned-up
objects seems like a bad idea.  Of course the close code still looks
after closing the vfds in the local backend.)

2.  If you want to hand the file over to some other process and exit,
you probably want to avoid race conditions or extra IPC burden.  To
achieve that you could 'pin' the file, so that it survives even while
not open in any backend.

3.  If the recount reaches zero when you 'release' and the file isn't
'pinned', then you must delete the underlying files.

4.  When the DSM segment is detached, we spin through all associated
shared files that we're still 'attached' to (ie opened but didn't
release) and decrement the refcount.  If any shared file's refcount
reaches zero its files should be deleted, even if was 'pinned'.

In other words, the associated DSM segment's lifetime is the maximum
lifetime of shared files, but it can be shorter if you 'release' in
all backends and don't 'pin'.  It's up to client code can come up with
some scheme to make that work, if it doesn't take the easy route of
pinning until DSM segment destruction.

I think in your case you'd simply pin all the BufFiles allowing
workers to exit 

Re: [HACKERS] possibility to specify template database for pg_regress

[Pavel Stehule]

2017-02-08 8:30 GMT+01:00 Michael Paquier :

> On Wed, Feb 8, 2017 at 4:24 PM, Pavel Stehule 
> wrote:
> > What is sense for list of databases?
>
> ECPG uses it for example, see 0992259.
>
> > Some option --template can be great - with backpatch if it is possible.
>
> That's not really complicated to patch... That could be a nice task
> for a starter.
>

Today I am doing some training - I can look on it at evening

Regards

Pavel


> --
> Michael
>

Re: [HACKERS] possibility to specify template database for pg_regress

[Michael Paquier]

On Wed, Feb 8, 2017 at 4:24 PM, Pavel Stehule  wrote:
> What is sense for list of databases?

ECPG uses it for example, see 0992259.

> Some option --template can be great - with backpatch if it is possible.

That's not really complicated to patch... That could be a nice task
for a starter.
-- 
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] possibility to specify template database for pg_regress

[Pavel Stehule]

2017-02-08 1:30 GMT+01:00 Michael Paquier :

> On Wed, Feb 8, 2017 at 9:23 AM, Tom Lane  wrote:
> > I ran into a use-case just today: I wanted to run one particular
> > regression test script under CLOBBER_CACHE_ALWAYS, but it needed
> > stuff created by earlier scripts, and I didn't especially want to
> > run all of those scripts under CCA.  With a way to select a template,
> > I could've run the earlier scripts in a normal build, renamed the
> > ending-state regression database to something else, and then installed
> > a CCA-enabled executable and run a test with just the script of
> > interest.  The way I actually got it done was considerably hackier :-(
>
> Looking at the code, --dbname can actually accept a list of databases.
> Perhaps we could just have the equivalent for templates? I think that
> we just need to be sure that the template list matches the length of
> the database list if the template list is longer than one.
>

What is sense for list of databases?

Some option --template can be great - with backpatch if it is possible.

Regards

Pavel



> --
> Michael
>

Re: [HACKERS] Parallel bitmap heap scan

[Dilip Kumar]

On Wed, Feb 8, 2017 at 8:07 AM, Robert Haas  wrote:

.Thanks for your input, I have few queries about these comments.

> 2. Add a new function dsa_pointer tbm_prepare_shared_iterate(TIDBitmap
> *tbm) which allocates shared iteration arrays using the DSA passed to
> tbm_create() and returns a dsa_pointer to the result.  Arrange this so
> that if it's called more than once, each call returns a separate
> iterator, so that you can call it once to get the main iterator and a
> second time for the prefetch iterator, but have both of those point to
> the same underlying iteration arrays.
>
> 3. Add a new function TBMSharedIterator
> *tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer) which is called
> once per backend and gets passed the dsa_pointer from the previous
> step.

IIUC, tbm_prepare_shared_iterate will be called only by the leader,
for tbmiterator as well as for the prefetch_iterator. And,
tbm_attach_shared_iterate will be called by each backend and for both
the iterators.

IMHO, tbm_attach_shared_iterate should return TBMIterator with
reference to TBMSharedIterator inside it. The reason behind same is
that we can not keep TBMIterateResult inside TBMSharedIterator
otherwise, results will also go in shared memory but we want to have
local result memory for each worker so that other worker doesn't
disturb it.

Another option can be that we change the tbm_shared_iterate as explained below

TBMIterateResult * tbm_shared_iterate(TBMSharedIterator *,
TBMIterateResult *result).

Now, if result passed to this API is NULL then it will allocate the
memory for the result and that way we will have local result memory,
and if it's not NULL we will use this memory to store our results.
BitmapHeapNode already having a reference to the TBMIterateResult so
we should not have any problem in passing this reference to the
tbm_shared_iterate. I think this one looks better than what I
explained above.

Please suggest.

>
> 4. Add a new function TBMIterateResult
> *tbm_shared_iterate(TBMSharedIterator *) to fetch the next result.


-- 
Regards,
Dilip Kumar
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] DROP SUBSCRIPTION and ROLLBACK

[Masahiko Sawada]

On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier
 wrote:
> On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao  wrote:
>> On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek
>>  wrote:
>>> For example what happens if apply crashes during the DROP
>>> SUBSCRIPTION/COMMIT and is not started because the delete from catalog
>>> is now visible so the subscription is no longer there?
>>
>> Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, i.e.,
>> make it emit an error if it's executed within user's transaction block.
>
> It seems to me that this is exactly Petr's point: using
> PreventTransactionChain() to prevent things to happen.

Agreed. It's better to prevent to be executed inside user transaction
block. And I understood there is too many failure scenarios we need to
handle.

>> Also DROP SUBSCRIPTION should call CommitTransactionCommand() just
>> after removing the entry from pg_subscription, then connect to the publisher
>> and remove the replication slot.
>
> For consistency that may be important.

Agreed.

Attached patch, please give me feedback.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


drop_subscription_and_rollback_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] WAL consistency check facility

[Kuntal Ghosh]

On Tue, Jan 31, 2017 at 9:36 PM, Robert Haas  wrote:
> On Thu, Jan 5, 2017 at 12:54 AM, Kuntal Ghosh
>  wrote:
>> I've attached the patch with the modified changes. PFA.
>
> Can this patch check contrib/bloom?
>
Added support for generic resource manager type.

> +/*
> + * Mask some line pointer bits, particularly those marked as
> + * used on a master and unused on a standby.
> + */
>
> Comment doesn't explain why we need to do this.
>
Added comments.

> +/*
> + * For GIN_DELETED page, the page is initialized to empty.
> + * Hence mask everything.
> + */
> +if (opaque->flags & GIN_DELETED)
> +memset(page_norm, MASK_MARKER, BLCKSZ);
> +else
> +mask_unused_space(page_norm);
>
> If the page is initialized to empty, why do we need to mask
> anything/everything?  Anyway, it doesn't seem right to mask the
> GinPageOpaque structure itself. Or at least it doesn't seem right to
> mask the flags word.
>
Modified accordingly.

>
> +if (!HeapTupleHeaderXminFrozen(page_htup))
> +page_htup->t_infomask |= HEAP_XACT_MASK;
> +else
> +page_htup->t_infomask |= HEAP_XMAX_COMMITTED |
> HEAP_XMAX_INVALID;
>
> Comment doesn't address this logic.  Also, the "else" case shouldn't
> exist at all, I think.
Added comments. I think that "Else" part is needed for xmax.

>
> +/*
> + * For a speculative tuple, the content of t_ctid is conflicting
> + * between the backup page and current page. Hence, we set it
> + * to the current block number and current offset.
> + */
>
> Why does it differ?  Is that a bug?
>
Added comments.

>
> +/*
> + * During replay of a btree page split, we don't set the BTP_SPLIT_END
> + * flag of the right sibling and initialize th cycle_id to 0 for the same
> + * page.
> + */
>
> A reference to the name of the redo function might be helpful here
> (and in some other places), unless it's just ${AMNAME}_redo.  "th" is
> a typo for "the".
Corrected.

> +appendStringInfoString(buf, " (full page
> image, apply)");
> +else
> +appendStringInfoString(buf, " (full page image)");
>
> How about "(full page image)" and "(full page image, for WAL verification)"?
>
> Similarly in XLogDumpDisplayRecord, I think we should assume that
> "FPW" means what it has always meant, and leave that output alone.
> Instead, distinguish the WAL-consistency-checking case when it
> happens, e.g. "(FPW for consistency check)".
>
Corrected.

> +checkConsistency(XLogReaderState *record)
>
> How about CheckXLogConsistency()?
>
> + * If needs_backup is true or wal consistency check is enabled for
>
> ...or WAL checking is enabled...
>
> + * If WAL consistency is enabled for the resource manager of
>
> If WAL consistency checking is enabled...
>
> + * Note: when a backup block is available in XLOG with BKPIMAGE_APPLY flag
>
> with the BKPIMAGE_APPLY flag
Modified accordingly.

>
> - * In RBM_ZERO_* modes, if the page doesn't exist, the relation is extended
> - * with all-zeroes pages up to the referenced block number.  In
> - * RBM_ZERO_AND_LOCK and RBM_ZERO_AND_CLEANUP_LOCK modes, the return value
> + * In RBM_ZERO_* modes, if the page doesn't exist or BKPIMAGE_APPLY flag
> + * is not set for the backup block, the relation is extended with all-zeroes
> + * pages up to the referenced block number.
>
> OK, I'm puzzled by this.  Surely we don't want the WAL consistency
> checking facility to cause the relation to be extended like this.  And
> I don't see why it would, because the WAL consistency checking happens
> after the page changes have already been applied.  I wonder if this
> hunk is in error and should be dropped.
>
Dropped comments.

> +PageXLogRecPtrSet(phdr->pd_lsn, PG_UINT64_MAX);
> +phdr->pd_prune_xid = PG_UINT32_MAX;
> +phdr->pd_flags |= PD_PAGE_FULL | PD_HAS_FREE_LINES;
> +phdr->pd_flags |= PD_ALL_VISIBLE;
> +#define MASK_MARKER0xFF
> (and many others)
>
> Why do we mask by setting bits rather than clearing bits?  My
> intuition would have been to zero things we want to mask, rather than
> setting them to one.
>
Modified accordingly so that we can zero things during masking instead
of setting them to one.

> +{
> +newwalconsistency[i] = true;
> +}
>
> Superfluous braces.
>
Corrected.

> +/*
> + * Leave if no masking functions defined, this is possible in the case
> + * resource managers generating just full page writes, comparing an
> + * image to itself has no meaning in those cases.
> + */
> +if (RmgrTable[rmid].rm_mask == NULL)
> +return;
>
> ...and also...
>
> +/*
> + * This feature is enabled only for the resource managers 

Re: [HACKERS] [PATCH] configure-time knob to set default ssl ciphers

[Tom Lane]

Peter Eisentraut  writes:
> On 2/7/17 11:21 AM, Tom Lane wrote:
>> A compromise that might be worth considering is to introduce
>> #define PG_DEFAULT_SSL_CIPHERS "HIGH:MEDIUM:+3DES:!aNULL"
>> into pg_config_manual.h, which would at least give you a reasonably
>> stable target point for a long-lived patch.

> You'd still need to patch postgresql.conf.sample somehow.

Right.  The compromise position that I had in mind was to add the
#define in pg_config_manual.h and teach initdb to propagate it into
the installed copy of postgresql.conf, as we've done with other GUCs
with platform-dependent defaults, such as backend_flush_after.

That still leaves the question of what to do with the SGML docs.
We could add some weasel wording to the effect that the default might
be platform-specific, or we could leave the docs alone and expect the
envisioned Red Hat patch to patch config.sgml along with
pg_config_manual.h.

It looks like the xxx_flush_after GUCs aren't exactly fully documented
as to this point, so we have some work to do there 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

Re: [HACKERS] [PATCH] configure-time knob to set default ssl ciphers

[Peter Eisentraut]

On 2/7/17 11:21 AM, Tom Lane wrote:
> A compromise that might be worth considering is to introduce
> 
> #define PG_DEFAULT_SSL_CIPHERS "HIGH:MEDIUM:+3DES:!aNULL"
> 
> into pg_config_manual.h, which would at least give you a reasonably
> stable target point for a long-lived patch.

You'd still need to patch postgresql.conf.sample somehow.

-- 
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] drop support for Python 2.3

[Tom Lane]

Peter Eisentraut  writes:
> I would like to propose that we drop support for Python 2.3.
> ...
> We do have buildfarm coverage on prairiedog.  However, that runs a >10
> year old operating system, so I think it is not representing real usage.

I have no particular objection to dropping 2.3 support, but should we
make some effort to fail gracefully (ie, with a relevant error message)
on older versions?  I would guess that the effect of your patch will be
to produce quite opaque failures.  We seem to be computing python_version
in configure, so it shouldn't be that hard to check.

> - It's unlikely that Python 2.3 is still used in practice.  Python 2.4
> is in RHEL 5, which is the typically the oldest mainstream OS we look at.

Hm, is there anything running 2.4 in the buildfarm?  If we're going to
claim support for 2.4, we'd be well advised to 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] pg_bsd_indent: implement -lps ("leave preprocessor space")

[Tom Lane]

Piotr Stefaniak  writes:
> this is a patch that Andres asked me for. It makes pg_bsd_indent leave
> preprocessor space alone, as in this example:

> #if 0
> #  if 0
> # if 0
> #   error
> # endif
> #  endif
> #else
> #  line 7
> #endif

Um ... but the point of pgindent is to standardize spacing, not to let
people invent their own style.  If you wanted to have a discussion about
whether pgindent should force preprocessor directives to look like the
above, we could talk about that.  But I do not want to be reading code that
looks like the above in one place and code that does not ten lines away.

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] drop support for Python 2.3

[Peter Eisentraut]

I would like to propose that we drop support for Python 2.3.  Reasons:

- Python 3.6 was released in December.  The list of versions we need to
manage is growing.

- Older Python versions are increasingly hard to build locally for testing.

- It's unlikely that Python 2.3 is still used in practice.  Python 2.4
is in RHEL 5, which is the typically the oldest mainstream OS we look at.

- We could remove some cruft from the code.

We do have buildfarm coverage on prairiedog.  However, that runs a >10
year old operating system, so I think it is not representing real usage.

Patch attached.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 94592edd240680224e5a971e8be7a5127f193d0c Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 16 Aug 2016 12:00:00 -0400
Subject: [PATCH] Drop support for Python 2.3

---
 .../hstore_plpython/expected/hstore_plpython.out   |  8 ++--
 contrib/hstore_plpython/sql/hstore_plpython.sql|  8 ++--
 doc/src/sgml/installation.sgml |  6 +-
 src/pl/plpython/expected/plpython_ereport.out  | 24 +-
 src/pl/plpython/plpy_typeio.c  | 10 ++---
 src/pl/plpython/sql/plpython_ereport.sql   |  8 ++--
 6 files changed, 19 insertions(+), 45 deletions(-)

diff --git a/contrib/hstore_plpython/expected/hstore_plpython.out b/contrib/hstore_plpython/expected/hstore_plpython.out
index b0025c04a8..df49cd5f37 100644
--- a/contrib/hstore_plpython/expected/hstore_plpython.out
+++ b/contrib/hstore_plpython/expected/hstore_plpython.out
@@ -6,9 +6,7 @@ LANGUAGE plpythonu
 TRANSFORM FOR TYPE hstore
 AS $$
 assert isinstance(val, dict)
-i = list(val.items())
-i.sort()
-plpy.info(i)
+plpy.info(sorted(val.items()))
 return len(val)
 $$;
 SELECT test1('aa=>bb, cc=>NULL'::hstore);
@@ -24,9 +22,7 @@ LANGUAGE plpython2u
 TRANSFORM FOR TYPE hstore
 AS $$
 assert isinstance(val, dict)
-i = list(val.items())
-i.sort()
-plpy.info(i)
+plpy.info(sorted(val.items()))
 return len(val)
 $$;
 SELECT test1n('aa=>bb, cc=>NULL'::hstore);
diff --git a/contrib/hstore_plpython/sql/hstore_plpython.sql b/contrib/hstore_plpython/sql/hstore_plpython.sql
index d55bedaf50..911bbd67fe 100644
--- a/contrib/hstore_plpython/sql/hstore_plpython.sql
+++ b/contrib/hstore_plpython/sql/hstore_plpython.sql
@@ -7,9 +7,7 @@ CREATE FUNCTION test1(val hstore) RETURNS int
 TRANSFORM FOR TYPE hstore
 AS $$
 assert isinstance(val, dict)
-i = list(val.items())
-i.sort()
-plpy.info(i)
+plpy.info(sorted(val.items()))
 return len(val)
 $$;
 
@@ -22,9 +20,7 @@ CREATE FUNCTION test1n(val hstore) RETURNS int
 TRANSFORM FOR TYPE hstore
 AS $$
 assert isinstance(val, dict)
-i = list(val.items())
-i.sort()
-plpy.info(i)
+plpy.info(sorted(val.items()))
 return len(val)
 $$;
 
diff --git a/doc/src/sgml/installation.sgml b/doc/src/sgml/installation.sgml
index 4431ed75a9..231d9dea4a 100644
--- a/doc/src/sgml/installation.sgml
+++ b/doc/src/sgml/installation.sgml
@@ -193,11 +193,7 @@ 
   language, you need a Python
   installation with the header files and
   the distutils module.  The minimum
-  required version is Python 2.3.
-  (To work with function arguments of type numeric, a 2.3.x
-  installation must include the separately-available cdecimal
-  module; note the PL/Python regression tests
-  will not pass if that is missing.)
+  required version is Python 2.4.
   Python 3 is supported if it's
   version 3.1 or later; but see
   PL/Python documentation]]>
diff --git a/src/pl/plpython/expected/plpython_ereport.out b/src/pl/plpython/expected/plpython_ereport.out
index 13bd0ab335..1dafd94c72 100644
--- a/src/pl/plpython/expected/plpython_ereport.out
+++ b/src/pl/plpython/expected/plpython_ereport.out
@@ -94,26 +94,22 @@ kwargs = {
 "column_name": _column_name, "datatype_name": _datatype_name,
 "constraint_name": _constraint_name
 }
-# ignore None values - should work on Python2.3
-dict = {}
-for k in kwargs:
-if kwargs[k] is not None:
-dict[k] = kwargs[k]
-plpy.error(**dict)
+# ignore None values
+plpy.error(**dict((k, v) for k, v in iter(kwargs.items()) if v))
 $$ LANGUAGE plpythonu;
 SELECT raise_exception('hello', 'world');
 ERROR:  plpy.Error: hello
 DETAIL:  world
 CONTEXT:  Traceback (most recent call last):
-  PL/Python function "raise_exception", line 13, in 
-plpy.error(**dict)
+  PL/Python function "raise_exception", line 9, in 
+plpy.error(**dict((k, v) for k, v in iter(kwargs.items()) if v))
 PL/Python function "raise_exception"
 SELECT 

Re: [HACKERS] PUBLICATIONS and pg_dump

[Stephen Frost]

Peter,

On Tue, Feb 7, 2017 at 22:49 Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 2/7/17 3:19 PM, Stephen Frost wrote:
> > I understand that this is a bit complicated, but I would have thought
> > we'd do something similar to what is done for DEFAULT PRIVILEGES, where
> > we include the "global" default privileges when we are doing a dump of
> > "everything", but if we're dumping a specific schema then we only
> > include the default privileges directly associated with that schema.
> >
> > Perhaps we need to include publications which are specific to a
> > particular table, but the current logic of, essentially, "always include
> > all publications" does not seem to make a lot of sense to me.
>
> I think it would be sensible to refine it along those lines.


Great!  I've added it to the open items list for PG10.

Thanks!

Stephen

Re: [HACKERS] PUBLICATIONS and pg_dump

[Peter Eisentraut]

On 2/7/17 3:19 PM, Stephen Frost wrote:
> I understand that this is a bit complicated, but I would have thought
> we'd do something similar to what is done for DEFAULT PRIVILEGES, where
> we include the "global" default privileges when we are doing a dump of
> "everything", but if we're dumping a specific schema then we only
> include the default privileges directly associated with that schema.
> 
> Perhaps we need to include publications which are specific to a
> particular table, but the current logic of, essentially, "always include
> all publications" does not seem to make a lot of sense to me.

I think it would be sensible to refine it along those lines.

-- 
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] Idea on how to simplify comparing two sets

[Merlin Moncure]

1On Tue, Feb 7, 2017 at 9:46 PM, Joel Jacobson  wrote:
> On Tue, Feb 7, 2017 at 4:58 PM, Tom Lane  wrote:
>> Joel Jacobson  writes:
>>> Currently there is no simple way to check if two sets are equal.
>>
>> Uh ... maybe check whether SELECT set1 EXCEPT SELECT set2
>> and SELECT set2 EXCEPT SELECT set1 are both empty?
>
> Yes, that's one way, but it's ugly as you have to repeat yourself and
> write both sets two times.
> Not an issue for small queries, but if you have two big queries stored
> in a .sql file,
> you would have to modify both places for each query and always make
> sure they are identical.

A CTE might help:

WITH left AS (something complex),
right AS (something complex)
SELECT COUNT(*) = 0 AS good FROM
(
  SELECT * FROM left EXCEPT SELECT * FROM right
  UNION ALL
  SELECT * FROM right EXCEPT SELECT * FROM left
) q;

This isn't the most efficient solution, but is easily abstracted into
dynamic SQL (meaning, you could pass both queries as arguments to a
checker function).  Another, similar approach is to abstract the query
behind a view which ISTM is a practice you are underutilizing based on
your comments :-).

If I were in a hurry and the dataset was enormous I would probably
dump both queries identically ordered to a .csv, and do:
diff left.csv right.csv | head -1

in bash or something like that.  Not sure if the utility of a
bidirectional EXCEPT is enough to justify adding custom syntax for
that approach.  I use the 'double EXCEPT' tactic fairly often and
understand the need, but the bar for non-standard syntax is pretty
high (and has been getting higher over the years, I think).

merlin


-- 
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] Parallel bitmap heap scan

[Robert Haas]

On Sun, Feb 5, 2017 at 9:48 AM, Dilip Kumar  wrote:
> Here are the latest version of the patch, which address all the
> comments given by Robert.

I think it would be useful to break the remaining patch into two
parts, one that enhances the tidbitmap.c API and another that uses
that to implement Parallel Bitmap Heap Scan.  In this review I'm going
to focus on the changes to tidbitmap.c and tidbitmap.h.

+extern void tbm_restore_shared_members(TIDBitmap *tbm,
+   ParallelTBMInfo * stbm);
+extern void tbm_update_shared_members(TIDBitmap *tbm,
+  ParallelTBMInfo * parallel_tbm);
+voidtbm_set_parallel(TIDBitmap *tbm, void *area);
+extern Size tbm_estimate_parallel_tbminfo(Size size);
+TIDBitmap  *tbm_attach(ParallelTBMInfo * parallel_tbm, void *area);
+TBMIterator *tbm_begin_shared_iterate(TIDBitmap *tbm,
+ ParallelTBMInfo * parallel_tbm, bool prefetch);
+TBMIterateResult *tbm_shared_iterate(TBMIterator *iterator);
+voidtbm_init_shared_iterator(ParallelTBMInfo * tbminfo);

There are several cosmetic problems here.  You may have noticed that
all function prototypes in PostgreSQL header files are explicitly
declared extern; yours should be, too.  Also, there is extra
whitespace before some of the variable names here, like
"ParallelTBMInfo * tbminfo" instead of "ParallelTBMInfo *tbminfo". If
that is due to pgindent, the solution is to add the typedef names to
your local typedef list.  Also, tbm_restore_shared_members doesn't
actually exist.

More broadly, this seems like an extremely complicated API.  Even
ignoring the function that doesn't exist, that's still 7 different
functions just for shared iteration, which seems like a lot.  I
suggest the following API:

1. Add an additional argument to tbm_create(), dsa_area *dsa.  If it's
NULL, we allocate a backend-private memory for the hash entries as
normal.  If it's true, we use the dsa_area to store the hash entries,
using the infrastructure added by your 0002 and revised in
c3c4f6e1740be256178cd7551d5b8a7677159b74.  (You can use a flag in the
BitmapIndexScan and BitmapOr to decide whether to pass NULL or
es_query_dsa.)

2. Add a new function dsa_pointer tbm_prepare_shared_iterate(TIDBitmap
*tbm) which allocates shared iteration arrays using the DSA passed to
tbm_create() and returns a dsa_pointer to the result.  Arrange this so
that if it's called more than once, each call returns a separate
iterator, so that you can call it once to get the main iterator and a
second time for the prefetch iterator, but have both of those point to
the same underlying iteration arrays.

3. Add a new function TBMSharedIterator
*tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer) which is called
once per backend and gets passed the dsa_pointer from the previous
step.

4. Add a new function TBMIterateResult
*tbm_shared_iterate(TBMSharedIterator *) to fetch the next result.

As compared with your proposal, this involves only 3 functions instead
of 7 (plus one new argument to another function), and I think it's a
lot clearer what those functions are actually doing.  You don't need
tbm_estimate_parallel_tbminfo() any more because the data being passed
from one backend to another is precisely a dsa_pointer, and the bitmap
scan can just leave space for that in its own estimator function.  You
don't need tbm_update_shared_members() separately from
tbm_begin_shared_iterate() separately from tbm_init_shared_iterator()
because tbm_prepare_shared_iterate() can do all that stuff.  You don't
need tbm_set_parallel() because the additional argument to
tbm_create() takes care of that need.

The way you've got ParallelTBMInfo set up right now doesn't respect
the separation of concerns between nodeBitmapHeapscan.c and
tidbitmap.c properly. tidbitmap.c shouldn't know that the caller
intends on creating two iterators, one of which is for prefetching.
The design above hopefully addresses that: each call to
tbm_prepare_shared_iterate returns a dsa_pointer to a separate chunk
of shared iterator state, but tidbitmap.c does not need to know how
many times that will be called.

Apart from the above, this patch will need a rebase over today's
commits, and please make sure all functions you add have high-quality
header comments.

Thanks,

-- 
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] Press Release Draft - 2016-02-09 Cumulative Update

[Jonathan S. Katz]

> On Feb 7, 2017, at 6:40 PM, Alvaro Herrera  wrote:
> 
> Jonathan S. Katz wrote:
>> 
>>> On Feb 7, 2017, at 4:07 PM, Alvaro Herrera  wrote:
>>> 
>>> Jonathan S. Katz wrote:
>>> 
 Thanks for the clarification.  I have updated the recipe along with Emre’s 
 comments here:
 
 [updated text not included in the email]
>>> 
>>> I still don't think the recipe is a very good one because it leaves you
>>> with a window where the affected columns are not indexed at all.
>> 
>> Okay, so I propose two options:
>> 
>>  1.  Does anyone have a recipe they recommend that might be better? OR
> 
> Do the CREATE INDEX CONCURRENTLY first, then DROP INDEX CONCURRENTLY the
> old index, then rename the new index to the old name.

Cool.  Updated:

https://git.postgresql.org/gitweb/?p=press.git;a=blob;f=update_releases/current/20170209updaterelease.txt;h=56551b56eb1f70e734c5762abf5ddf6bd6965fdb;hb=c1b7f9c0cfd0ff106409d14a36c3122a8ee460d0
 


I added a note to alert people to disk space usage utilizing this method.

Thanks for the help!

Jonathan.

[HACKERS] pg_bsd_indent: implement -lps ("leave preprocessor space")

[Piotr Stefaniak]

Hello,

this is a patch that Andres asked me for. It makes pg_bsd_indent leave
preprocessor space alone, as in this example:

#if 0
#  if 0
#   if 0
# error
#   endif
#  endif
#else
#  line 7
#endif
diff -ur pg_bsd_indent/args.c pg_bsd_indent_patched/args.c
--- pg_bsd_indent/args.c	2014-01-31 04:09:31.0 +0100
+++ pg_bsd_indent_patched/args.c	2017-02-08 01:59:01.921080544 +0100
@@ -221,6 +221,9 @@
 		"lc", PRO_INT, 0, 0, _comment_max_col
 	},
 	{
+		"lps", PRO_BOOL, false, ON, _preprocessor_space
+	},
+	{
 		"lp", PRO_BOOL, true, ON, _to_parens
 	},
 	{
@@ -269,6 +272,9 @@
 		"nip", PRO_BOOL, true, OFF, _parameters
 	},
 	{
+		"nlps", PRO_BOOL, false, OFF, _preprocessor_space
+	},
+	{
 		"nlp", PRO_BOOL, true, OFF, _to_parens
 	},
 	{
diff -ur pg_bsd_indent/indent.c pg_bsd_indent_patched/indent.c
--- pg_bsd_indent/indent.c	2014-01-31 04:06:43.0 +0100
+++ pg_bsd_indent_patched/indent.c	2017-02-08 01:56:59.039931984 +0100
@@ -1091,17 +1091,25 @@
 			(s_code != e_code))
 dump_line();
 			*e_lab++ = '#';	/* move whole line to 'label' buffer */
+			if (leave_preprocessor_space) {
+while (isblank((int)*buf_ptr)) {
+	CHECK_SIZE_LAB;
+	*e_lab++ = *buf_ptr++;
+}
+			}
+			else {
+while (isblank((int)*buf_ptr)) {
+	buf_ptr++;
+}
+			}
+			t_ptr = e_lab;
+
 			{
 int in_comment = 0;
 int com_start = 0;
 charquote = 0;
 int com_end = 0;
 
-while (*buf_ptr == ' ' || *buf_ptr == '\t') {
-	buf_ptr++;
-	if (buf_ptr >= buf_end)
-		fill_buffer();
-}
 while (*buf_ptr != '\n' || in_comment) {
 	CHECK_SIZE_LAB;
 	*e_lab = *buf_ptr++;
@@ -1179,7 +1187,7 @@
 ps.pcase = false;
 			}
 
-			if (strncmp(s_lab, "#if", 3) == 0) {
+			if (t_ptr[0] == 'i' && t_ptr[1] == 'f') {
 if (blanklines_around_conditional_compilation) {
 	int c;
 	prefix_blankline_requested++;
@@ -1192,7 +1200,7 @@
 } else
 	diag(1, "#if stack overflow");
 			} else
-if (strncmp(s_lab, "#else", 5) == 0) {
+if (t_ptr[0] == 'e' && t_ptr[1] == 'l') {
 	if (ifdef_level <= 0)
 		diag(1, "Unmatched #else");
 	else {
@@ -1200,7 +1208,7 @@
 		ps = state_stack[ifdef_level - 1];
 	}
 } else
-	if (strncmp(s_lab, "#endif", 6) == 0) {
+	if (strncmp(t_ptr, "endif", 5) == 0) {
 		if (ifdef_level <= 0)
 			diag(1, "Unmatched #endif");
 		else {
diff -ur pg_bsd_indent/indent_globs.h pg_bsd_indent_patched/indent_globs.h
--- pg_bsd_indent/indent_globs.h	2005-11-15 01:30:24.0 +0100
+++ pg_bsd_indent_patched/indent_globs.h	2017-02-08 01:57:51.003994806 +0100
@@ -222,6 +222,7 @@
 	 * "for(e;e;e)" should be indented an extra
 	 * tab stop so that they don't conflict with
 	 * the code that follows */
+EXTERN int leave_preprocessor_space;
 
 /* -troff font state information */
 

-- 
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] possibility to specify template database for pg_regress

[Michael Paquier]

On Wed, Feb 8, 2017 at 9:23 AM, Tom Lane  wrote:
> I ran into a use-case just today: I wanted to run one particular
> regression test script under CLOBBER_CACHE_ALWAYS, but it needed
> stuff created by earlier scripts, and I didn't especially want to
> run all of those scripts under CCA.  With a way to select a template,
> I could've run the earlier scripts in a normal build, renamed the
> ending-state regression database to something else, and then installed
> a CCA-enabled executable and run a test with just the script of
> interest.  The way I actually got it done was considerably hackier :-(

Looking at the code, --dbname can actually accept a list of databases.
Perhaps we could just have the equivalent for templates? I think that
we just need to be sure that the template list matches the length of
the database list if the template list is longer than one.
-- 
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] possibility to specify template database for pg_regress

[Andres Freund]

On 2017-02-07 19:23:45 -0500, Tom Lane wrote:
> Michael Paquier  writes:
> > On Wed, Feb 8, 2017 at 12:43 AM, Pavel Stehule  
> > wrote:
> >> Is possible to specify template database for pg_regress?
> >> I have to run tests on database with thousands database objects. Using
> >> template is much faster than import these objects.
> 
> > Not directly, all the databases created by pg_regress are enforced
> > with template0.. Having a switch sounds useful though without seeing
> > in details your use case.
> 
> I ran into a use-case just today: I wanted to run one particular
> regression test script under CLOBBER_CACHE_ALWAYS, but it needed
> stuff created by earlier scripts, and I didn't especially want to
> run all of those scripts under CCA.  With a way to select a template,
> I could've run the earlier scripts in a normal build, renamed the
> ending-state regression database to something else, and then installed
> a CCA-enabled executable and run a test with just the script of
> interest.  The way I actually got it done was considerably hackier :-(

Can't you do that with --use-existing?  I'm pretty sure I used it for
very similar issues before.  And yes, the --help text for that is
misleading.

- 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] Press Release Draft - 2016-02-09 Cumulative Update

[Alvaro Herrera]

Jonathan S. Katz wrote:
> 
> > On Feb 7, 2017, at 4:07 PM, Alvaro Herrera  wrote:
> > 
> > Jonathan S. Katz wrote:
> > 
> >> Thanks for the clarification.  I have updated the recipe along with Emre’s 
> >> comments here:
> >> 
> >> [updated text not included in the email]
> > 
> > I still don't think the recipe is a very good one because it leaves you
> > with a window where the affected columns are not indexed at all.
> 
> Okay, so I propose two options:
> 
>   1.  Does anyone have a recipe they recommend that might be better? OR

Do the CREATE INDEX CONCURRENTLY first, then DROP INDEX CONCURRENTLY the
old index, then rename the new index to the old name.

-- 
Álvaro Herrerahttps://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] possibility to specify template database for pg_regress

[Tom Lane]

Michael Paquier  writes:
> On Wed, Feb 8, 2017 at 12:43 AM, Pavel Stehule  
> wrote:
>> Is possible to specify template database for pg_regress?
>> I have to run tests on database with thousands database objects. Using
>> template is much faster than import these objects.

> Not directly, all the databases created by pg_regress are enforced
> with template0.. Having a switch sounds useful though without seeing
> in details your use case.

I ran into a use-case just today: I wanted to run one particular
regression test script under CLOBBER_CACHE_ALWAYS, but it needed
stuff created by earlier scripts, and I didn't especially want to
run all of those scripts under CCA.  With a way to select a template,
I could've run the earlier scripts in a normal build, renamed the
ending-state regression database to something else, and then installed
a CCA-enabled executable and run a test with just the script of
interest.  The way I actually got it done was considerably hackier :-(

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] possibility to specify template database for pg_regress

[Michael Paquier]

On Wed, Feb 8, 2017 at 12:43 AM, Pavel Stehule  wrote:
> Is possible to specify template database for pg_regress?
> I have to run tests on database with thousands database objects. Using
> template is much faster than import these objects.

Not directly, all the databases created by pg_regress are enforced
with template0.. Having a switch sounds useful though without seeing
in details your use case.
-- 
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] DROP SUBSCRIPTION and ROLLBACK

[Michael Paquier]

On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao  wrote:
> On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek
>  wrote:
>> For example what happens if apply crashes during the DROP
>> SUBSCRIPTION/COMMIT and is not started because the delete from catalog
>> is now visible so the subscription is no longer there?
>
> Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, i.e.,
> make it emit an error if it's executed within user's transaction block.

It seems to me that this is exactly Petr's point: using
PreventTransactionChain() to prevent things to happen.

> Also DROP SUBSCRIPTION should call CommitTransactionCommand() just
> after removing the entry from pg_subscription, then connect to the publisher
> and remove the replication slot.

For consistency that may be important.
-- 
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] Caching index AM working data across aminsert calls

[Robert Haas]

On Tue, Feb 7, 2017 at 6:04 PM, Tom Lane  wrote:
> It's always been possible for index AMs to cache data across successive
> amgettuple calls within a single SQL command: the IndexScanDesc.opaque
> field is meant for precisely that.  However, no comparable facility
> exists for amortizing setup work across successive aminsert calls.
> The attached proposed patch adds such a feature and teaches gin,
> gist, and brin to use it.  (The other standard index AMs keep everything
> they need in the relcache, so there's little to improve there.)
>
> The improvement I see from this is fairly modest in a normal build.
> In an example similar to the gin regression test's main insert query,
>
> insert into gin_test_tbl select array[1, 2, g] from generate_series(1, 
> 100) g;
>
> the overall insertion speed increases perhaps 10%, which is nice but
> not great.  gist and brin are less, maybe 5% or so.

I think that's more than nice.  I think it's great.  It's not that
easy to squeeze 5-10% out of common operations.

(I have not reviewed the patch.)

-- 
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] Caching index AM working data across aminsert calls

[Tom Lane]

It's always been possible for index AMs to cache data across successive
amgettuple calls within a single SQL command: the IndexScanDesc.opaque
field is meant for precisely that.  However, no comparable facility
exists for amortizing setup work across successive aminsert calls.
The attached proposed patch adds such a feature and teaches gin,
gist, and brin to use it.  (The other standard index AMs keep everything
they need in the relcache, so there's little to improve there.)

The improvement I see from this is fairly modest in a normal build.
In an example similar to the gin regression test's main insert query,

insert into gin_test_tbl select array[1, 2, g] from generate_series(1, 100) 
g;

the overall insertion speed increases perhaps 10%, which is nice but
not great.  gist and brin are less, maybe 5% or so.  However, because
most of what happens in the saved work is catalog lookups, the savings
in a CLOBBER_CACHE_ALWAYS test build are pretty substantial: the runtime
of the gin regression script, on my workstation, goes from 40 minutes
to 4 seconds.  (Yes, really.)  The gist and brin test scripts are less
insert-heavy but still lose several minutes apiece.  Since the core
regression tests are run multiple times (twice serially and once in
parallel) in the standard buildfarm cycle, I estimate that this patch
would cut over 1.5 hours from the cycle time for a CLOBBER_CACHE_ALWAYS
critter running on hardware similar to mine.  I think that alone makes it
worth doing.

The reason this has been hard up to now is that the aminsert function
is not passed any useful place to cache per-statement data.  What I
chose to do in the attached is to add suitable fields to struct IndexInfo
and pass that to aminsert.  That's not widening the index AM API very
much because IndexInfo is already within the ken of ambuild.  I figured
that this might be a particularly useful way to do it because it means
that aminsert also has access to the other data in the IndexInfo struct,
which might save it having to recompute some things.  And it makes the
DDL info available to ambuild and aminsert more similar, which seems good
on general principles.

I also looked into the idea of using the index relcache entry's
rd_amcache field for this purpose, but that fails immediately in a
CLOBBER_CACHE_ALWAYS build, because gininsert (at least, probably the
other ones too) is not robust against its GinState disappearing from under
it mid-insert.  Since rd_amcache goes away on a cache flush even if the
index is open, that doesn't work.  We could maybe fix that by introducing
some way for AMs to control the lifetime of rd_amcache, but it would
result in a substantially more complex and invasive patch than this one,
and I'm unconvinced that it'd be worth the trouble.

Thoughts, objections?

regards, tom lane

diff --git a/contrib/bloom/blinsert.c b/contrib/bloom/blinsert.c
index 29bc5ce..913f1f8 100644
*** a/contrib/bloom/blinsert.c
--- b/contrib/bloom/blinsert.c
*** blbuildempty(Relation index)
*** 190,196 
   */
  bool
  blinsert(Relation index, Datum *values, bool *isnull,
! 		 ItemPointer ht_ctid, Relation heapRel, IndexUniqueCheck checkUnique)
  {
  	BloomState	blstate;
  	BloomTuple *itup;
--- 190,198 
   */
  bool
  blinsert(Relation index, Datum *values, bool *isnull,
! 		 ItemPointer ht_ctid, Relation heapRel,
! 		 IndexUniqueCheck checkUnique,
! 		 IndexInfo *indexInfo)
  {
  	BloomState	blstate;
  	BloomTuple *itup;
diff --git a/contrib/bloom/bloom.h b/contrib/bloom/bloom.h
index a75a235..39d8d05 100644
*** a/contrib/bloom/bloom.h
--- b/contrib/bloom/bloom.h
*** extern bool blvalidate(Oid opclassoid);
*** 189,195 
  /* index access method interface functions */
  extern bool blinsert(Relation index, Datum *values, bool *isnull,
  		 ItemPointer ht_ctid, Relation heapRel,
! 		 IndexUniqueCheck checkUnique);
  extern IndexScanDesc blbeginscan(Relation r, int nkeys, int norderbys);
  extern int64 blgetbitmap(IndexScanDesc scan, TIDBitmap *tbm);
  extern void blrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
--- 189,196 
  /* index access method interface functions */
  extern bool blinsert(Relation index, Datum *values, bool *isnull,
  		 ItemPointer ht_ctid, Relation heapRel,
! 		 IndexUniqueCheck checkUnique,
! 		 struct IndexInfo *indexInfo);
  extern IndexScanDesc blbeginscan(Relation r, int nkeys, int norderbys);
  extern int64 blgetbitmap(IndexScanDesc scan, TIDBitmap *tbm);
  extern void blrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
diff --git a/doc/src/sgml/indexam.sgml b/doc/src/sgml/indexam.sgml
index 5d8e557..9afd7f6 100644
*** a/doc/src/sgml/indexam.sgml
--- b/doc/src/sgml/indexam.sgml
*** aminsert (Relation indexRelation,
*** 259,265 
bool *isnull,
ItemPointer heap_tid,
Relation heapRelation,
!   IndexUniqueCheck checkUnique);
  
 Insert a new tuple into an existing index. 

Re: [HACKERS] Press Release Draft - 2016-02-09 Cumulative Update

[Jonathan S. Katz]

> On Feb 7, 2017, at 4:39 PM, Michael Banck  wrote:
> 
> Hi,
> 
> Am Dienstag, den 07.02.2017, 15:58 -0500 schrieb Jonathan S. Katz:
> 
> 
>> https://git.postgresql.org/gitweb/?p=press.git;a=blob;f=update_releases/current/20170209updaterelease.txt;h=f90d4716f240dbea4cca647b099f79865545b633;hb=d85498c284275bcab4752b91476834de780648b8
> 
> It says "[...]then rows that were updated by transactions running at the
> same time as the CREATE INDEX CONCURRENTLY command could have been index
> incorrectly."
> 
> That sounds off to me, shouldn't it be "indexed incorrectly" or
> something?

Yes, passive voice :(  I’ve made the modification on my local copy and will 
push it up after the resolution on the CREATE INDEX recipe.

Jonathan

-- 
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] Press Release Draft - 2016-02-09 Cumulative Update

[Jonathan S. Katz]

> On Feb 7, 2017, at 4:07 PM, Alvaro Herrera  wrote:
> 
> Jonathan S. Katz wrote:
> 
>> Thanks for the clarification.  I have updated the recipe along with Emre’s 
>> comments here:
>> 
>> [updated text not included in the email]
> 
> I still don't think the recipe is a very good one because it leaves you
> with a window where the affected columns are not indexed at all.

Okay, so I propose two options:

1.  Does anyone have a recipe they recommend that might be better? OR
2.  We just leave out the recipe altogether (which is what I am leaning 
towards at the moment).

Thanks!

Jonathan

-- 
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] Parallel bitmap heap scan

[Robert Haas]

On Tue, Feb 7, 2017 at 4:45 PM, Andres Freund  wrote:
> On 2017-02-07 16:36:55 -0500, Robert Haas wrote:
>> On Tue, Feb 7, 2017 at 4:15 PM, Andres Freund  wrote:
>> > FWIW, I think it'd have been better to not add the new callbacks as
>> > parameters to *_create(), but rather have them be "templatized" like the
>> > rest of simplehash.  That'd require that callback to check the context,
>> > to know whether it should use shared memory or not, but that seems fine
>> > to me.  Right now this pushes the head of simplehash above a
>> > cacheline...
>>
>> Something like the attached?
>
> Yes.
>
>> +#ifndef SH_USE_NONDEFAULT_ALLOCATOR
>> +
>
> That should probably be documented in the file header.

Right.  OK, did that and a few other cleanups, and committed.

-- 
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] Parallel bitmap heap scan

[Andres Freund]

On 2017-02-07 16:36:55 -0500, Robert Haas wrote:
> On Tue, Feb 7, 2017 at 4:15 PM, Andres Freund  wrote:
> > FWIW, I think it'd have been better to not add the new callbacks as
> > parameters to *_create(), but rather have them be "templatized" like the
> > rest of simplehash.  That'd require that callback to check the context,
> > to know whether it should use shared memory or not, but that seems fine
> > to me.  Right now this pushes the head of simplehash above a
> > cacheline...
> 
> Something like the attached?

Yes.

> +#ifndef SH_USE_NONDEFAULT_ALLOCATOR
> +

That should probably be documented in the file header.


Thanks!

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] Press Release Draft - 2016-02-09 Cumulative Update

[Michael Banck]

Hi,

Am Dienstag, den 07.02.2017, 15:58 -0500 schrieb Jonathan S. Katz:


> https://git.postgresql.org/gitweb/?p=press.git;a=blob;f=update_releases/current/20170209updaterelease.txt;h=f90d4716f240dbea4cca647b099f79865545b633;hb=d85498c284275bcab4752b91476834de780648b8

It says "[...]then rows that were updated by transactions running at the
same time as the CREATE INDEX CONCURRENTLY command could have been index
incorrectly."

That sounds off to me, shouldn't it be "indexed incorrectly" or
something?


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer




-- 
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] Parallel bitmap heap scan

[Robert Haas]

On Tue, Feb 7, 2017 at 4:15 PM, Andres Freund  wrote:
> FWIW, I think it'd have been better to not add the new callbacks as
> parameters to *_create(), but rather have them be "templatized" like the
> rest of simplehash.  That'd require that callback to check the context,
> to know whether it should use shared memory or not, but that seems fine
> to me.  Right now this pushes the head of simplehash above a
> cacheline...

Something like the attached?

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


rework-simplehash-allocator.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] Parallel bitmap heap scan

[Robert Haas]

On Tue, Feb 7, 2017 at 4:13 PM, Jeff Janes  wrote:
> I'm getting compiler errors:
>
> In file included from execGrouping.c:47:
> ../../../src/include/lib/simplehash.h:91: error: redefinition of typedef
> 'simplehash_allocate'
> ../../../src/include/lib/simplehash.h:91: note: previous declaration of
> 'simplehash_allocate' was here
> ../../../src/include/lib/simplehash.h:92: error: redefinition of typedef
> 'simplehash_free'
> ../../../src/include/lib/simplehash.h:92: note: previous declaration of
> 'simplehash_free' was here

Thanks, I'll stick an #ifdef guard around that.

-- 
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] Parallel bitmap heap scan

[Andres Freund]

On 2017-02-07 13:13:43 -0800, Jeff Janes wrote:
> On Tue, Feb 7, 2017 at 1:03 PM, Robert Haas  wrote:
>
> > On Sun, Feb 5, 2017 at 9:48 AM, Dilip Kumar  wrote:
> > >> I think maybe we should rename the functions to element_allocate,
> > >> element_free, and element_allocator_ctx, or something like that.  The
> > >> current names aren't capitalized consistently with other things in
> > >> this module, and putting the word "element" in there would make it
> > >> more clear what the purpose of this thing is.
> > >
> > > Fixed as per the suggestion
> >
> > Eh, not really.  You changed the memory context to be called
> > element_allocator_ctx, rather than changing the args passed to the
> > element allocator to have that name, which is what I had in mind.
> >
> > I did some assorted renaming and other cosmetic improvements and committed
> > 0002.
> >
>
> I'm getting compiler errors:
>
> In file included from execGrouping.c:47:
> ../../../src/include/lib/simplehash.h:91: error: redefinition of typedef
> 'simplehash_allocate'
> ../../../src/include/lib/simplehash.h:91: note: previous declaration of
> 'simplehash_allocate' was here
> ../../../src/include/lib/simplehash.h:92: error: redefinition of typedef
> 'simplehash_free'
> ../../../src/include/lib/simplehash.h:92: note: previous declaration of
> 'simplehash_free' was here

Oh yea, that too - you can't just redefine a typedef like that according
to C89 :(


-- 
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] Parallel bitmap heap scan

[Andres Freund]

On 2017-02-07 16:03:43 -0500, Robert Haas wrote:
> On Sun, Feb 5, 2017 at 9:48 AM, Dilip Kumar  wrote:
> >> I think maybe we should rename the functions to element_allocate,
> >> element_free, and element_allocator_ctx, or something like that.  The
> >> current names aren't capitalized consistently with other things in
> >> this module, and putting the word "element" in there would make it
> >> more clear what the purpose of this thing is.
> >
> > Fixed as per the suggestion
> 
> Eh, not really.  You changed the memory context to be called
> element_allocator_ctx, rather than changing the args passed to the
> element allocator to have that name, which is what I had in mind.
> 
> I did some assorted renaming and other cosmetic improvements and committed 
> 0002.

FWIW, I think it'd have been better to not add the new callbacks as
parameters to *_create(), but rather have them be "templatized" like the
rest of simplehash.  That'd require that callback to check the context,
to know whether it should use shared memory or not, but that seems fine
to me.  Right now this pushes the head of simplehash above a
cacheline...

I'm also doubtful about the naming of the default callbacks:
+/* default memory allocator function */
+static void *
+SH_DEFAULT_ALLOC(Size size, void *args)
+{
+   MemoryContext context = (MemoryContext) args;
+
+   return MemoryContextAllocExtended(context, size,
+ MCXT_ALLOC_HUGE | MCXT_ALLOC_ZERO);
+}

SH_DEFAULT_ALLOC sounds like it's a name that's one of the prefixed
names (like SH_CREATE actually becomes pagetable_create and such) - but
afaics it's not.  Which afaics means that this'll generate symbol
conflicts if one translation unit uses multiple simplehash.h style
hashes.  Afaics these should either be prefixed, or static inline
functions.

- 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] Parallel bitmap heap scan

[Jeff Janes]

On Tue, Feb 7, 2017 at 1:03 PM, Robert Haas  wrote:

> On Sun, Feb 5, 2017 at 9:48 AM, Dilip Kumar  wrote:
> >> I think maybe we should rename the functions to element_allocate,
> >> element_free, and element_allocator_ctx, or something like that.  The
> >> current names aren't capitalized consistently with other things in
> >> this module, and putting the word "element" in there would make it
> >> more clear what the purpose of this thing is.
> >
> > Fixed as per the suggestion
>
> Eh, not really.  You changed the memory context to be called
> element_allocator_ctx, rather than changing the args passed to the
> element allocator to have that name, which is what I had in mind.
>
> I did some assorted renaming and other cosmetic improvements and committed
> 0002.
>

I'm getting compiler errors:

In file included from execGrouping.c:47:
../../../src/include/lib/simplehash.h:91: error: redefinition of typedef
'simplehash_allocate'
../../../src/include/lib/simplehash.h:91: note: previous declaration of
'simplehash_allocate' was here
../../../src/include/lib/simplehash.h:92: error: redefinition of typedef
'simplehash_free'
../../../src/include/lib/simplehash.h:92: note: previous declaration of
'simplehash_free' was here


gcc version 4.4.7 20120313 (Red Hat 4.4.7-17) (GCC)

Cheers,

Jeff

Re: [HACKERS] Press Release Draft - 2016-02-09 Cumulative Update

[Alvaro Herrera]

Jonathan S. Katz wrote:

> Thanks for the clarification.  I have updated the recipe along with Emre’s 
> comments here:
>
> [updated text not included in the email]

I still don't think the recipe is a very good one because it leaves you
with a window where the affected columns are not indexed at all.

-- 
Álvaro Herrerahttps://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: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

This was my previous understanding of ON_ERROR_STOP. Somewhere in the
course of developing this patch I lost that. Glad to have it back.

The only changes I made were to invalid booleans on if/elif, and the final
branch balancing check won't set status to EXIT_USER unless it's
non-interactive and ON_ERROR_STOP = on.


About v10: Patch applies, make check ok, psql tap test ok. Html doc 
generation ok.


Everything looks ok to me.

Interactive tests behave as expected, especially ctrl-C and with 
on_error_stop=1.


ISTM that everything has been addressed.

I've switched the patch to "Ready for Committers", let's what happens on 
their side...


--
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] Parallel bitmap heap scan

[Robert Haas]

On Sun, Feb 5, 2017 at 9:48 AM, Dilip Kumar  wrote:
>> I think maybe we should rename the functions to element_allocate,
>> element_free, and element_allocator_ctx, or something like that.  The
>> current names aren't capitalized consistently with other things in
>> this module, and putting the word "element" in there would make it
>> more clear what the purpose of this thing is.
>
> Fixed as per the suggestion

Eh, not really.  You changed the memory context to be called
element_allocator_ctx, rather than changing the args passed to the
element allocator to have that name, which is what I had in mind.

I did some assorted renaming and other cosmetic improvements and committed 0002.

-- 
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] Press Release Draft - 2016-02-09 Cumulative Update

[Jonathan S. Katz]

> On Feb 7, 2017, at 12:44 PM, Alvaro Herrera  wrote:
> 
> Jonathan S. Katz wrote:
> 
>> Below is the draft of the press release for the update this Thursday:
>> 
>> https://git.postgresql.org/gitweb/?p=press.git;a=blob;f=update_releases/current/20170209updaterelease.md;h=0cccb8986c08527f65f13d704a78c36bb8de7fef;hb=afc01091dea8a1597e8e21430edc3908c581ce0c
>>  
>> 
>> 
>> As there are a lot of updates I did my best to consolidate some of the 
>> bullet points and as usual, people are directed to the release notes.  
>> Please let me know if there are any inaccuracies so I can fix them ASAP.
> 
> Please do post the proposed text on list for ease of review.  I wasn't
> looking at the text, so I wouldn't have noticed this if Emre hadn't
> replied:
> 
>  76 If you believe you have been affected by the aforementioned CREATE INDEX 
> CONCURRENTLY bug, you will have to rebuild the index.  An example of 
> rebuilding an index:
>  77 
>  78 BEGIN;
>  79 DROP INDEX bad_index_name;
>  80 CREATE INDEX CONCURRENTLY bad_index_name ON table_name (column_name); 
> /* replace names with your original index definition */
>  81 COMMIT;
> 
> This is not a good recipe, because using CREATE INDEX CONCURRENTLY in
> the same transaction that grabs an exclusive lock on the table for the
> DROP INDEX is pointless -- the access exclusive lock is held until the
> end of the transaction, and CIC does not work inside a transaction
> anyway so it'd raise an ERROR and rollback the DROP INDEX.  So the user
> would probably drop the BEGIN/COMMIT sequence in order for this to work
> in the first place.  (The other option is to use CREATE INDEX not
> concurrent, but that would lock the table for a very long time).

Thanks for the clarification.  I have updated the recipe along with Emre’s 
comments here:

https://git.postgresql.org/gitweb/?p=press.git;a=blob;f=update_releases/current/20170209updaterelease.txt;h=f90d4716f240dbea4cca647b099f79865545b633;hb=d85498c284275bcab4752b91476834de780648b8
 


Thanks!

Jonathan

Re: [HACKERS] Press Release Draft - 2016-02-09 Cumulative Update

[Jonathan S. Katz]

> On Feb 7, 2017, at 11:25 AM, Emre Hasegeli  wrote:
> 
>> As there are a lot of updates I did my best to consolidate some of the
>> bullet points and as usual, people are directed to the release notes.
>> Please let me know if there are any inaccuracies so I can fix them ASAP.
> 
> Just some minor points:
> 
>> * Several fixes for PostgreSQL operating in hot standby mode
> 
> It sounded unnatural to me.  Maybe it is better without "PostgreSQL".
> 
>> * Several vacuum and autovacuum fxies
> 
> Typo
> 
>> * Several Unicode fixes
> 
> It sounded alarming to me.  I see just one related item on the release
> notes.  Maybe we can clarify the problem.
> 
>> * Sync our copy of the timezone library with IANA release tzcode2016j
> 
> This is repeated on the following sentence.
> 
>> BEGIN;
>> DROP INDEX bad_index_name;
>> CREATE INDEX CONCURRENTLY bad_index_name ON table_name (column_name); /* 
>> replace names with your original index definition */
>> COMMIT;
> 
> Maybe you meant CREATE INDEX without CONCURRENTLY?

Thanks for the corrections / suggestions.  I have applied them and will push a 
new version shortly!

-- 
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] GSoC 2017 - LAST CALL

[Alexander Korotkov]

Hi!

On Tue, Feb 7, 2017 at 4:39 PM, Stephen Frost  wrote:

> I will be submitting our application to Google tomorrow morning (the
> deadline is Feb 9th, 1600 UTC, but I'd like to do it a bit early to
> have time to address any issues).  Please get your project ideas
> submitted so we can include them.
>

Thank you for reminder.  I added couple more project ideas and added me,
Oleg and Teodor to possible mentors of some projects.
That seems to be all that I have for now.

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

[HACKERS] PUBLICATIONS and pg_dump

[Stephen Frost]

Peter,

* Peter Eisentraut (pete...@gmx.net) wrote:
> Logical replication
> 
> - Add PUBLICATION catalogs and DDL
> - Add SUBSCRIPTION catalog and DDL
> - Define logical replication protocol and output plugin
> - Add logical replication workers

I think we need to have a bit more discussion regarding where
publications (and maybe subscriptions... not sure on that though) fit
when it comes to pg_dump.

In particular, I'm trying to clean up the pg_dump TAP tests and am
finding things I wouldn't have expected.  For example, publications
appear to be included in pretty much every pg_dump output, no matter if
a specific schema or even table was explicitly called for, or if that
publication or subscription was explicitly associated with that table.

The example I'm playing with is:

CREATE PUBLICATION pub2 WITH (PUBLISH INSERT, PUBLISH UPDATE, PUBLISH
DELETE);

and a simple:

pg_dump -n public -t t1

Will end up including the CREATE PUBLICATION command.

In fact, I'm not entirely sure how to have it not included in pg_dump's
output.

I understand that this is a bit complicated, but I would have thought
we'd do something similar to what is done for DEFAULT PRIVILEGES, where
we include the "global" default privileges when we are doing a dump of
"everything", but if we're dumping a specific schema then we only
include the default privileges directly associated with that schema.

Perhaps we need to include publications which are specific to a
particular table, but the current logic of, essentially, "always include
all publications" does not seem to make a lot of sense to me.

I'm happy to be corrected if I've grossly misunderstood something here,
of course.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Cache Hash Index meta page.

[Robert Haas]

On Tue, Feb 7, 2017 at 1:52 PM, Mithun Cy  wrote:
> On Tue, Feb 7, 2017 at 11:21 PM, Erik Rijkers  wrote:
>> On 2017-02-07 18:41, Robert Haas wrote:
>>>
>>> Committed with some changes (which I noted in the commit message).
>
> Thanks, Robert and all who have reviewed the patch and given their
> valuable comments.
>
>> This has caused a warning with gcc 6.20:
>>
>> hashpage.c: In function ‘_hash_getcachedmetap’:
>> hashpage.c:1245:20: warning: ‘cache’ may be used uninitialized in this
>> function [-Wmaybe-uninitialized]
>> rel->rd_amcache = cache;
>> ^~~
>
> Yes, I also see the warning.  I think the compiler is not able to see
> cache is always initialized and used under condition if
> (rel->rd_amcache == NULL).
> I think to make the compiler happy we can initialize the cache with
> NULL when it is defined.

Thanks for the reports and patch.  Committed.

-- 
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] Cache Hash Index meta page.

[Mithun Cy]

On Tue, Feb 7, 2017 at 11:21 PM, Erik Rijkers  wrote:
> On 2017-02-07 18:41, Robert Haas wrote:
>>
>> Committed with some changes (which I noted in the commit message).

Thanks, Robert and all who have reviewed the patch and given their
valuable comments.

> This has caused a warning with gcc 6.20:
>
> hashpage.c: In function ‘_hash_getcachedmetap’:
> hashpage.c:1245:20: warning: ‘cache’ may be used uninitialized in this
> function [-Wmaybe-uninitialized]
> rel->rd_amcache = cache;
> ^~~

Yes, I also see the warning.  I think the compiler is not able to see
cache is always initialized and used under condition if
(rel->rd_amcache == NULL).
I think to make the compiler happy we can initialize the cache with
NULL when it is defined.

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com


cache_metap_compiler_warning01
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: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
>
> It seems that ON_ERROR_STOP is mostly ignored by design when in
> interactive mode, probably because it is nicer not to disconnect the user
> who is actually typing things on a terminal.
>
> """
>   ON_ERROR_STOP
>
> By default, command processing continues after an error. When this
> variable is set to on, processing will instead stop immediately. In
> interactive mode, psql will return to the command prompt; otherwise, psql
> will exit, returning error code 3 to distinguish this case from fatal error
> conditions, which are reported using error code 1.
> """
>

This was my previous understanding of ON_ERROR_STOP. Somewhere in the
course of developing this patch I lost that. Glad to have it back.

The only changes I made were to invalid booleans on if/elif, and the final
branch balancing check won't set status to EXIT_USER unless it's
non-interactive and ON_ERROR_STOP = on.

> \if true
new \if is true, executing commands
> \endif
exited \if, executing commands
> \if false
new \if is false, ignoring commands until next \elif, \else, or \endif
> \endif
exited \if, executing commands
> \if error
unrecognized value "error" for "\if ": boolean expected
new \if is invalid, ignoring commands until next \endif
> \echo foo
inside inactive branch, command ignored.
> ^C
escaped \if, executing commands
> \echo foo
foo
> \endif
encountered un-matched \endif
>
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..c0ba4c4 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,67 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+SELECT
+EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer,
+EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee
+\gset
+\if :is_customer
+SELECT * FROM customer WHERE customer_id = 123;
+\elif :is_employee
+\echo 'is not a customer but is an employee'
+SELECT * FROM employee WHERE employee_id = 456;
+\else
+\if yes
+\echo 'not a customer or employee'
+\else
+\echo 'this should never print'
+\endif
+\endif
+
+
+Conditional blocks must begin with a \if and end
+with an \endif, and the pairs must be found in
+the same source file. If an EOF is reached on the main file or an
+\include-ed file before all local
+\if-\endif are matched, then
+psql will raise an error.
+
+
+A conditional block can have any number of 
+\elif clauses, which may optionally be followed by a
+single \else clause.
+
+
+The \if and \elif commands
+read the rest of the line and evaluate it as a boolean expression.
+Currently, expressions are limited to a single unquoted string
+which are evaluated like other on/off options, so the valid values
+are any unambiguous case insensitive matches for one of:
+true, false, 
1,
+0, on, off,
+yes, no.  So 
+t, T, and tR
+will all match true.
+
+
+Lines within false branches are not evaluated in any way: queries are
+not sent to the server, non-conditional commands are not evaluated but
+bluntly ignored, nested if-expressions in such branches are also not
+evaluated but are tallied to check for proper nesting.
+
+
+  
 
   
 \ir or \include_relative 
filename
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index c53733f..7a418c6 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -61,8 +61,16 @@ uninstall:
 
 clean distclean:
rm -f psql$(X) $(OBJS) lex.backup
+   rm -rf tmp_check
 
 # files removed here are supposed to be in the distribution tarball,
 # so do not clean them in the clean/distclean rules
 maintainer-clean: distclean
rm -f sql_help.h sql_help.c psqlscanslash.c
+
+
+check:
+   $(prove_check)
+
+installcheck:
+   $(prove_installcheck)
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index f17f610..63ddf0a 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -49,6 +49,7 @@
 #include "psqlscanslash.h"
 #include "settings.h"
 #include "variables.h"
+#include "fe_utils/psqlscan_int.h"
 
 /*
  * Editable database object types.
@@ -132,7 +133,7 @@ HandleSlashCmds(PsqlScanState scan_state,
status = PSQL_CMD_ERROR;
}
 
-   if (status != PSQL_CMD_ERROR)
+   if (status != PSQL_CMD_ERROR && pset.active_branch)
{
/* eat any remaining arguments after a valid command */
/* note we suppress evaluation of backticks here */
@@ -194,6 +195,30 @@ read_connect_arg(PsqlScanState scan_state)
return result;
 }
 
+/*
+ * Read 

Re: [HACKERS] Idea on how to simplify comparing two sets

[Fabien COELHO]

Currently there is no simple way to check if two sets are equal.


My 0.02€:

Assuming that you mean set = relation, and that there is a key (which 
should be the case for a set otherwise tuples cannot be distinguished, so 
this is not really a set), and assuming not null other data, then:


CREATE TABLE TAB1(k INT PRIMARY KEY, data TEXT NOT NULL);
INSERT INTO TAB1 VALUES (1, 'one'), (2, 'two'), (3, 'three');

CREATE TABLE TAB2(k INT PRIMARY KEY, data TEXT NOT NULL);
INSERT INTO TAB2 VALUES (1, 'one'), (2, 'deux'), (4, 'four');

The TAB1 to TAB2 difference is computed with:

SELECT
  CASE WHEN t1.k IS NULL THEN 'INSERT'
   WHEN t2.k IS NULL THEN 'DELETE'
   ELSE 'UPDATE'
  END AS operation,
  COALESCE(t1.k, t2.k) AS key
FROM TAB1 AS t1
  FULL JOIN TAB2 AS t2 USING (k)
WHERE
  t1.data IS NULL OR t2.data IS NULL OR t1.data <> t2.data;

Results in:

   UPDATE | 2
   DELETE | 3
   INSERT | 4

If there is no differences, then sets are equals...

If there is no associated data, a simpler condition:

  WHERE t1.k IS NULL OR t2.k IS NULL;

--
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] Cache Hash Index meta page.

[Erik Rijkers]

On 2017-02-07 18:41, Robert Haas wrote:

Committed with some changes (which I noted in the commit message).


This has caused a warning with gcc 6.20:

hashpage.c: In function ‘_hash_getcachedmetap’:
hashpage.c:1245:20: warning: ‘cache’ may be used uninitialized in this 
function [-Wmaybe-uninitialized]

rel->rd_amcache = cache;
^~~

which hopefully can be prevented...

thanks,

Erik 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] Idea on how to simplify comparing two sets

[Tom Lane]

"David G. Johnston"  writes:
> On Tue, Feb 7, 2017 at 8:58 AM, Tom Lane  wrote:
>> Joel Jacobson  writes:
>>> Currently there is no simple way to check if two sets are equal.

>> Uh ... maybe check whether SELECT set1 EXCEPT SELECT set2
>> and SELECT set2 EXCEPT SELECT set1 are both empty?

> ​SELECT set1 FULL EXCEPT SELECT set2 ?
> Matches with the concept and syntax of "FULL JOIN"​.

Actually ... now that you mention full join, I believe this works:

select * from (select ...) s1 full join (select ...) s2
  on ((s1.*)=(s2.*)) where s1.* is distinct from s2.*;

> That said I'm not sure how much we want to go down this road on our own.
> It'd be nice to have when its needed but its not something that gets much
> visibility on these lists to suggest a large pent-up demand.

Yeah, if this isn't in the standard and not in other databases either,
that would seem to suggest that it's not a big requirement.

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] [COMMITTERS] pgsql: Release note updates.

[Pavan Deolasee]

On Tue, Feb 7, 2017 at 9:29 PM, Alvaro Herrera 
wrote:

> Alvaro Herrera wrote:
>
> >
> > Hmm.  Now that I think about it, it is probably possible to have a
> > transaction started before CIC that inserted a bunch of rows, and then
> > runs the UPDATE during the CIC race window.  Maybe there's a reason the
> > bug wouldn't hit in that case but I don't see it, and I'm not able to
> > test it right now to verify.
>
> Pavan adds that it's possible to have a transaction do INSERT while CIC
> is running, then some other transaction executes the UPDATE.
>
>
Just to elaborate on that, once a backend ends up with stale cached
bitmaps, AFAICS only a second relcache flush can correct that. This may not
come for long time. In fact since CIC is already holding a lock on the
table, I think second relcache flush will only happen at the end of phase
2. This can take a long time, especially for very large tables. In
meanwhile, the compromised backend can run many transactions with the stale
information. Those transactions can see not only the existing rows, but
also new rows inserted by other new but now committed-good transactions.

It's all theory and I haven't had time to try this out.

Thanks,
Pavan

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

Re: [HACKERS] Idea on how to simplify comparing two sets

[David G. Johnston]

On Tue, Feb 7, 2017 at 8:58 AM, Tom Lane  wrote:

> Joel Jacobson  writes:
> > Currently there is no simple way to check if two sets are equal.
>
> Uh ... maybe check whether SELECT set1 EXCEPT SELECT set2
> and SELECT set2 EXCEPT SELECT set1 are both empty?
>

​SELECT set1 FULL EXCEPT SELECT set2 ?

Matches with the concept and syntax of "FULL JOIN"​.

or

SELECT set1 XOR SELECT set2

That said I'm not sure how much we want to go down this road on our own.
It'd be nice to have when its needed but its not something that gets much
visibility on these lists to suggest a large pent-up demand.

IS NOT DISTINCT FROM doesn't imply bi-direction any better than EXCEPT does
... if we are going to add new syntax I'd say it should at least do that.

David J.

Re: [HACKERS] Press Release Draft - 2016-02-09 Cumulative Update

[Alvaro Herrera]

Jonathan S. Katz wrote:

> Below is the draft of the press release for the update this Thursday:
> 
> https://git.postgresql.org/gitweb/?p=press.git;a=blob;f=update_releases/current/20170209updaterelease.md;h=0cccb8986c08527f65f13d704a78c36bb8de7fef;hb=afc01091dea8a1597e8e21430edc3908c581ce0c
>  
> 
> 
> As there are a lot of updates I did my best to consolidate some of the bullet 
> points and as usual, people are directed to the release notes.  Please let me 
> know if there are any inaccuracies so I can fix them ASAP.

Please do post the proposed text on list for ease of review.  I wasn't
looking at the text, so I wouldn't have noticed this if Emre hadn't
 replied:

  76 If you believe you have been affected by the aforementioned CREATE INDEX 
CONCURRENTLY bug, you will have to rebuild the index.  An example of rebuilding 
an index:
  77 
  78 BEGIN;
  79 DROP INDEX bad_index_name;
  80 CREATE INDEX CONCURRENTLY bad_index_name ON table_name (column_name); 
/* replace names with your original index definition */
  81 COMMIT;

This is not a good recipe, because using CREATE INDEX CONCURRENTLY in
the same transaction that grabs an exclusive lock on the table for the
DROP INDEX is pointless -- the access exclusive lock is held until the
end of the transaction, and CIC does not work inside a transaction
anyway so it'd raise an ERROR and rollback the DROP INDEX.  So the user
would probably drop the BEGIN/COMMIT sequence in order for this to work
in the first place.  (The other option is to use CREATE INDEX not
concurrent, but that would lock the table for a very long time).

-- 
Álvaro Herrerahttps://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] Press Release Draft - 2016-02-09 Cumulative Update

[Alvaro Herrera]

Michael Banck wrote:
> Hi,
> 
> Am Dienstag, den 07.02.2017, 10:37 -0500 schrieb Jonathan S. Katz:
> 
> > Below is the draft of the press release for the update this Thursday:
> 
> About the CREATE INDEX CONCURRENTLY issue, I wonder whether Peter's
> amcheck extension[1] would catch that (for B-Tree indexes at least), and
> if that is the case, whether we could mention that to our users as
> guidance for how to check for index corruption?

"it does not verify that the target index is consistent with its heap
relation"

-- 
Álvaro Herrerahttps://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] Cache Hash Index meta page.

[Robert Haas]

On Wed, Feb 1, 2017 at 12:23 AM, Michael Paquier
 wrote:
> On Sun, Jan 29, 2017 at 6:13 PM, Mithun Cy  wrote:
>>> HashMetaPage _hash_getcachedmetap(Relation rel, Buffer *metabuf, bool
>>> force_refresh);
>>>
>>> If the cache is initialized and force_refresh is not true, then this
>>> just returns the cached data, and the metabuf argument isn't used.
>>> Otherwise, if *metabuf == InvalidBuffer, we set *metabuf to point to
>>> the metabuffer, pin and lock it, use it to set the cache, release the
>>> lock, and return with the pin still held.  If *metabuf !=
>>> InvalidBuffer, we assume it's pinned and return with it still pinned.
>>
>> Thanks, Robert I have made a new patch which tries to do same. Now I
>> think code looks less complicated.
>
> Moved to CF 2017-03.

Committed with some changes (which I noted in the commit message).

-- 
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] Idea on how to simplify comparing two sets

[Joel Jacobson]

On Tue, Feb 7, 2017 at 6:28 PM, David Fetter  wrote:
> This could be shortened further to the following if we ever implement
> DISTINCT for window functions, which might involve implementing
> DISTINCT via hashing more generally, which means hashable
> types...whee!
>
> SELECT array_agg(DISTINCT (a IS NOT NULL)::int + (b IS NOT NULL)::int) OVER 
> () = '{2}'
> FROM a FULL JOIN b ON ...

That's still a lot more syntax than just adding "IS NOT DISTINCT FROM"
in between the sets.

I just thought the general approach at looking for ways to express new
things in SQL,
without introducing new keywords, but instead rely on existing keywords but
that currently are syntax errors when used in some semantic way,
is an interesting approach to allow extending the SQL syntax without
breaking backwards compatibility.

Are there any historical examples of when this approach has been used
to make progress in PostgreSQL?


-- 
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] Idea on how to simplify comparing two sets

[David Fetter]

On Tue, Feb 07, 2017 at 09:10:17AM -0800, David Fetter wrote:
> On Tue, Feb 07, 2017 at 04:13:40PM +0100, Joel Jacobson wrote:
> >  Hi hackers,
> > 
> > Currently there is no simple way to check if two sets are equal.
> 
> Assuming that a and b each has at least one NOT NULL column, is this
> simple enough?  Based on nothing much, I'm assuming here that the IS
> NOT NULL test is faster than IS NULL, but you can flip that and change
> the array to {0} with identical effect.
> 
> WITH t AS (
> SELECT a AS a, b AS b, (a IS NOT NULL)::int + (b IS NOT NULL)::int AS ind
> FROM a FULL JOIN b ON ...
> )
> SELECT array_agg(DISTINCT ind) = '{2}'
> FROM t;

You don't actually need a and b in the inner target list.

WITH t AS (
SELECT (a IS NOT NULL)::int + (b IS NOT NULL)::int AS ind
FROM a FULL JOIN b ON ...
)
SELECT array_agg(DISTINCT ind) = '{2}'
FROM t;

This could be shortened further to the following if we ever implement
DISTINCT for window functions, which might involve implementing
DISTINCT via hashing more generally, which means hashable
types...whee!

SELECT array_agg(DISTINCT (a IS NOT NULL)::int + (b IS NOT NULL)::int) OVER () 
= '{2}'
FROM a FULL JOIN b ON ... 

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] Proposal : For Auto-Prewarm.

[Mithun Cy]

On Tue, Feb 7, 2017 at 6:11 PM, Beena Emerson  wrote:
>  Yes it would be better to have only one pg_prewarm worker as the loader is
> idle for the entire server run time after the initial load activity of few
> secs.
Sorry, that is again a bug in the code. The code to handle SIGUSR1
somehow got deleted before I submitted patch_03 and I failed to notice
same.
As in the code loader bgworker is waiting on the latch to know the
status of dump bgworker. Actually, the loader bgworker should exit
right after launching the dump bgworker. I will try to fix this and
other comments given by you in my next patch.

-- 
Thanks and Regards
Mithun C Y
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] Idea on how to simplify comparing two sets

[David Fetter]

On Tue, Feb 07, 2017 at 04:13:40PM +0100, Joel Jacobson wrote:
>  Hi hackers,
> 
> Currently there is no simple way to check if two sets are equal.

Assuming that a and b each has at least one NOT NULL column, is this
simple enough?  Based on nothing much, I'm assuming here that the IS
NOT NULL test is faster than IS NULL, but you can flip that and change
the array to {0} with identical effect.

WITH t AS (
SELECT a AS a, b AS b, (a IS NOT NULL)::int + (b IS NOT NULL)::int AS ind
FROM a FULL JOIN b ON ...
)
SELECT array_agg(DISTINCT ind) = '{2}'
FROM t;

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] [PATCH] SortSupport for macaddr type

[Brandur Leach]

And as a short follow up, I've moved the patch to the
current commit fest:

https://commitfest.postgresql.org/13/743/


On Sun, Feb 5, 2017 at 1:56 PM, Brandur Leach  wrote:

> Hi Julien,
>
> Thank you for taking the time to do this review, and my
> apologies for the very delayed response. I lost track of
> this work and have only jumped back into it today.
>
> Please find attached a new version of the patch with your
> feedback integrated. I've also rebased the patch against
> the current master and selected a new OID because my old
> one is now in use.
>
> > * you used macaddr_cmp_internal() function name, for uuid
> >   the same function is named uuid_internal_cmp().  Using
> >   the same naming pattern is probably better.
>
> I was a little split on this one! It's true that UUID uses
> `_internal_cmp`, but `_cmp_internal` is also used in a
> number of places like `enum`, `timetz`, and `network`. I
> don't have a strong feeling about it either way, so I've
> changed it to `_internal_cmp` to match UUID as you
> suggested.
>
> > * the function comment on macaddr_abbrev_convert()
> >   doesn't mention specific little-endian handling
>
> I tried to bake this into the comment text. Here are the
> relevant lines of the amended version:
>
> * Packs the bytes of a 6-byte MAC address into a Datum and treats it
> as an
> * unsigned integer for purposes of comparison. On a 64-bit machine,
> there
> * will be two zeroed bytes of padding. The integer is converted to
> native
> * endianness to facilitate easy comparison.
>
> > * "There will be two bytes of zero padding on the least
> >   significant end"
> >
> > "least significant bits" would be better
>
> Also done. Here is the amended version:
>
> * On a 64-bit machine, zero out the 8-byte datum and copy the 6 bytes
> of
> * the MAC address in. There will be two bytes of zero padding on the
> end
> * of the least significant bits.
>
> > * This patch will trigger quite a lot modifications after
> >   a pgindent run.  Could you try to run pgindent on mac.c
> >   before sending an updated patch?
>
> Good call! I've run the new version through pgindent.
>
> Let me know if you have any further feedback and/or
> suggestions. Thanks!
>
> Brandur
>
>
> On Wed, Sep 14, 2016 at 3:14 AM, Julien Rouhaud  > wrote:
>
>> On 26/08/2016 19:44, Brandur wrote:
>> > Hello,
>> >
>>
>> Hello,
>>
>> > I've attached a patch to add SortSupport for Postgres' macaddr which
>> has the
>> > effect of improving the performance of sorting operations for the type.
>> The
>> > strategy that I employ is very similar to that for UUID, which is to
>> create
>> > abbreviated keys by packing as many bytes from the MAC address as
>> possible into
>> > Datums, and then performing fast unsigned integer comparisons while
>> sorting.
>> >
>> > I ran some informal local benchmarks, and for cardinality greater than
>> 100k
>> > rows, I see a speed up on `CREATE INDEX` of roughly 25% to 65%. (For
>> those
>> > interested, I put a few more numbers into a small report here [2].)
>> >
>>
>> That's a nice improvement!
>>
>> > Admittedly, this is not quite as useful as speeding up sorting on a
>> more common
>> > data type like TEXT or UUID, but the change still seems like a useful
>> > performance improvement. I largely wrote it as an exercise to
>> familiarize
>> > myself with the Postgres codebase.
>> >
>> > I'll add an entry into the current commitfest as suggested by the
>> Postgres Wiki
>> > and follow up here with a link.
>> >
>> > Thanks, and if anyone has feedback or other thoughts, let me know!
>> >
>>
>> I just reviewed your patch.  It applies and compiles cleanly, and the
>> abbrev feature works as intended.  There's not much to say since this is
>> heavily inspired on the uuid SortSupport. The only really specific part
>> is in the abbrev_converter function, and I don't see any issue with it.
>>
>> I have a few trivial comments:
>>
>> * you used macaddr_cmp_internal() function name, for uuid the same
>> function is named uuid_internal_cmp().  Using the same naming pattern is
>> probably better.
>>
>> * the function comment on macaddr_abbrev_convert() doesn't mention
>> specific little-endian handling
>>
>> * "There will be two bytes of zero padding on the least significant end"
>>
>> "least significant bits" would be better
>>
>> * This patch will trigger quite a lot modifications after a pgindent
>> run.  Could you try to run pgindent on mac.c before sending an updated
>> patch?
>>
>> Best regards.
>>
>> --
>> Julien Rouhaud
>> http://dalibo.com - http://dalibo.org
>>
>
>

Re: [HACKERS] Press Release Draft - 2016-02-09 Cumulative Update

[Michael Banck]

Hi,

Am Dienstag, den 07.02.2017, 10:37 -0500 schrieb Jonathan S. Katz:

> Below is the draft of the press release for the update this Thursday:

About the CREATE INDEX CONCURRENTLY issue, I wonder whether Peter's
amcheck extension[1] would catch that (for B-Tree indexes at least), and
if that is the case, whether we could mention that to our users as
guidance for how to check for index corruption?


Michael

[1] https://github.com/petergeoghegan/amcheck
-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer




-- 
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] DROP SUBSCRIPTION and ROLLBACK

[Fujii Masao]

On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek
 wrote:
> On 07/02/17 13:10, Masahiko Sawada wrote:
>> Hi all,
>>
>> While testing logical replciation I found that if the transaction
>> issued DROP SUBSCRIPTION　rollbacks then the logical repliation stops
>> and the subscription can never be removed later. The document says
>> that the replication worker associated with the subscription will not
>> stop until after the transaction that issued this command has
>> committed but it doesn't work.
>
> Ah then the docs are wrong and should be fixed. Maybe we should not
> allow DROP SUBSCRIPTION inside transaction similarly to CREATE INDEX
> CONCURRENTLY.
>
>> The cause of this is that DropSubscription stops the apply worker and
>> drops corresponding replication slot on publisher side without waiting
>> for commit or rollback. The launcher process launches the apply worker
>> again but the launched worker will fail to start logical replication
>> because corresponding replication slot is already removed. And the
>> orphan subscription can not be removed later.
>>
>> I think the logical replication should not stop and the corresponding
>> replication slot and replication origin should not be removed until
>> the transaction commits.
>>
>> The solution for this I came up with is that the launcher process
>> stops the apply worker after DROP SUBSCRIPTION is committed rather
>> than DropSubscription does. And the apply worker drops replication
>> slot and replication origin before exits. Attached draft patch fixes
>> this issue.
>>
>
> I don't think we can allow the slot drop to be postponed. There is too
> many failure scenarios where we would leave the remote slot in the
> database and that's not acceptable IMHO.
>
> For example what happens if apply crashes during the DROP
> SUBSCRIPTION/COMMIT and is not started because the delete from catalog
> is now visible so the subscription is no longer there?

Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, i.e.,
make it emit an error if it's executed within user's transaction block.
Also DROP SUBSCRIPTION should call CommitTransactionCommand() just
after removing the entry from pg_subscription, then connect to the publisher
and remove the replication slot.

Regards,

-- 
Fujii Masao


-- 
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] Press Release Draft - 2016-02-09 Cumulative Update

[Emre Hasegeli]

> As there are a lot of updates I did my best to consolidate some of the
> bullet points and as usual, people are directed to the release notes.
> Please let me know if there are any inaccuracies so I can fix them ASAP.

Just some minor points:

>  * Several fixes for PostgreSQL operating in hot standby mode

It sounded unnatural to me.  Maybe it is better without "PostgreSQL".

>  * Several vacuum and autovacuum fxies

Typo

>  * Several Unicode fixes

It sounded alarming to me.  I see just one related item on the release
notes.  Maybe we can clarify the problem.

>  * Sync our copy of the timezone library with IANA release tzcode2016j

This is repeated on the following sentence.

>  BEGIN;
>  DROP INDEX bad_index_name;
>  CREATE INDEX CONCURRENTLY bad_index_name ON table_name (column_name); /* 
> replace names with your original index definition */
>  COMMIT;

Maybe you meant CREATE INDEX without CONCURRENTLY?


-- 
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] configure-time knob to set default ssl ciphers

[Tom Lane]

Pavel Raiskup  writes:
> PostgreSQL server uses 'HIGH:MEDIUM:+3DES:!aNULL' cipher set by default,
> but what Fedora would like to have is 'PROFILE=SYSTEM' (works with
> Fedora-patched OpenSSL, so please don't waste your time with checking this
> elsewhere).
> ...
> I'd like to propose the attached patch, so we could (without downstream
> patching) do
> $ ./configure ... --with-openssl-be-ciphers=PROFILE=SYSTEM

Meh.  This is pretty far from a complete patch: it introduces an
undocumented configure switch, and it changes the default value for a GUC
without fixing either the corresponding SGML documentation or the
postgresql.conf.sample line for it.

While it would surely be possible to build all the infrastructure to make
that work right, I'm not really sure that we want to carry around that
much baggage for a single-system hack.

A compromise that might be worth considering is to introduce

#define PG_DEFAULT_SSL_CIPHERS "HIGH:MEDIUM:+3DES:!aNULL"

into pg_config_manual.h, which would at least give you a reasonably
stable target point for a long-lived patch.

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] Idea on how to simplify comparing two sets

[Joel Jacobson]

On Tue, Feb 7, 2017 at 4:58 PM, Tom Lane  wrote:
> Joel Jacobson  writes:
>> Currently there is no simple way to check if two sets are equal.
>
> Uh ... maybe check whether SELECT set1 EXCEPT SELECT set2
> and SELECT set2 EXCEPT SELECT set1 are both empty?

Yes, that's one way, but it's ugly as you have to repeat yourself and
write both sets two times.
Not an issue for small queries, but if you have two big queries stored
in a .sql file,
you would have to modify both places for each query and always make
sure they are identical.


-- 
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] [COMMITTERS] pgsql: Release note updates.

[Alvaro Herrera]

Alvaro Herrera wrote:
> Tom Lane wrote:
> > Alvaro Herrera  writes:
> 
> > > May I suggest
> > 
> > > +  If CREATE INDEX CONCURRENTLY was used to build an index
> > > +  that depends on a column not previously indexed, then rows
> > > +  updated by transactions that ran concurrently with
> > > +  the CREATE INDEX command could have missed receiving
> > > +  index entries.
> > 
> > Can we say "pre-existing rows that were updated by...", or is that
> > too optimistic?
> 
> Hmm.  Now that I think about it, it is probably possible to have a
> transaction started before CIC that inserted a bunch of rows, and then
> runs the UPDATE during the CIC race window.  Maybe there's a reason the
> bug wouldn't hit in that case but I don't see it, and I'm not able to
> test it right now to verify.

Pavan adds that it's possible to have a transaction do INSERT while CIC
is running, then some other transaction executes the UPDATE.

-- 
Álvaro Herrerahttps://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] Idea on how to simplify comparing two sets

[Tom Lane]

Joel Jacobson  writes:
> Currently there is no simple way to check if two sets are equal.

Uh ... maybe check whether SELECT set1 EXCEPT SELECT set2
and SELECT set2 EXCEPT SELECT set1 are both empty?

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] possibility to specify template database for pg_regress

[Pavel Stehule]

Hi

Is possible to specify template database for pg_regress?

I have to run tests on database with thousands database objects. Using
template is much faster than import these objects.

Regards

Pavel

Re: [HACKERS] Idea on how to simplify comparing two sets

[Joel Jacobson]

But that's already a valid statement, so there is no ambiguity:

SELECT TRUE WHERE FALSE
IS NOT DISTINCT FROM
(SELECT TRUE);
 bool
--
(0 rows)


If you want to compare the set (SELECT TRUE WHERE FALSE) with the set
(SELECT TRUE) then just add parenthesis:
(SELECT TRUE WHERE FALSE)
IS NOT DISTINCT FROM
(SELECT TRUE);
ERROR:  syntax error at or near "IS"
LINE 2: IS NOT DISTINCT FROM
^

Which is currently invalid syntax.




On Tue, Feb 7, 2017 at 4:40 PM, Anders Granlund
 wrote:
> What about this ambiguity?
>
> SELECT TRUE
> WHERE FALSE
> IS NOT DISTINCT FROM
> (SELECT TRUE)
>
> On Tue, Feb 7, 2017 at 4:13 PM, Joel Jacobson  wrote:
>>
>>  Hi hackers,
>>
>> Currently there is no simple way to check if two sets are equal.
>>
>> Looks like no RDBMS in the world has a simple command for it.
>>
>> You have to do something like:
>>
>> WITH
>> T1 AS (SELECT * FROM Foo WHERE FooID BETWEEN 1 AND 1),
>> T2 AS (SELECT * FROM Bar WHERE BarID BETWEEN 1 AND 1)
>> SELECT
>> GREATEST(
>> (SELECT COUNT(*) FROM T1),
>> (SELECT COUNT(*) FROM T2)
>> )
>> =
>> (SELECT COUNT(*) FROM (
>> SELECT * FROM T1
>> INTERSECT ALL
>> SELECT * FROM T2
>> ) AS X)
>> INTO _Identical;
>>
>> or,
>>
>> SELECT 'Missmatch!' WHERE EXISTS (
>> SELECT * FROM Foo
>> FULL JOIN Bar ON (Foo.FooID = Bar.BarID AND
>>  Foo IS NOT DISTINCT FROM Bar)
>> WHERE TRUE
>> AND ( Foo.FooID BETWEEN 1 AND 1 AND
>>   Bar.BarID BETWEEN 1 AND 1)
>> AND ( Foo.FooID IS NULL OR
>>   Bar.BarID IS NULL);
>>
>> Introducing new SQL keywords is of course not an option,
>> since it would possibly break backwards compatibility.
>>
>> So here is an idea that doesn't break backwards compatibility:
>>
>> Let's give a meaning for the existing IS DISTINCT and IS NOT DISTINCT,
>> that is currently a syntax error when used between two sets.
>>
>> SELECT 1 IS DISTINCT FROM SELECT 1;
>> ERROR:  syntax error at or near "SELECT"
>> LINE 1: SELECT 1 IS DISTINCT FROM SELECT 1;
>>
>> The example above could be written as:
>>
>> _Identical := (
>> SELECT * FROM Foo WHERE FooID BETWEEN 1 AND 1
>> IS NOT DISTINCT FROM
>> SELECT * FROM Bar WHERE BarID BETWEEN 1 AND 1
>> );
>>
>> Which would set _Identical to TRUE if the two sets are equal,
>> and FALSE otherwise.
>>
>> Since it's currently a syntax error, there is no risk for changed
>> behaviour for any existing executable queries.
>>
>> Thoughts?
>>
>> /Joel
>>
>>
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>
>



-- 
Joel Jacobson

Mobile: +46703603801
Trustly.com | Newsroom | LinkedIn | Twitter


-- 
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] Idea on how to simplify comparing two sets

[Anders Granlund]

What about this ambiguity?

SELECT TRUE
WHERE FALSE
IS NOT DISTINCT FROM
(SELECT TRUE)

On Tue, Feb 7, 2017 at 4:13 PM, Joel Jacobson  wrote:

>  Hi hackers,
>
> Currently there is no simple way to check if two sets are equal.
>
> Looks like no RDBMS in the world has a simple command for it.
>
> You have to do something like:
>
> WITH
> T1 AS (SELECT * FROM Foo WHERE FooID BETWEEN 1 AND 1),
> T2 AS (SELECT * FROM Bar WHERE BarID BETWEEN 1 AND 1)
> SELECT
> GREATEST(
> (SELECT COUNT(*) FROM T1),
> (SELECT COUNT(*) FROM T2)
> )
> =
> (SELECT COUNT(*) FROM (
> SELECT * FROM T1
> INTERSECT ALL
> SELECT * FROM T2
> ) AS X)
> INTO _Identical;
>
> or,
>
> SELECT 'Missmatch!' WHERE EXISTS (
> SELECT * FROM Foo
> FULL JOIN Bar ON (Foo.FooID = Bar.BarID AND
>  Foo IS NOT DISTINCT FROM Bar)
> WHERE TRUE
> AND ( Foo.FooID BETWEEN 1 AND 1 AND
>   Bar.BarID BETWEEN 1 AND 1)
> AND ( Foo.FooID IS NULL OR
>   Bar.BarID IS NULL);
>
> Introducing new SQL keywords is of course not an option,
> since it would possibly break backwards compatibility.
>
> So here is an idea that doesn't break backwards compatibility:
>
> Let's give a meaning for the existing IS DISTINCT and IS NOT DISTINCT,
> that is currently a syntax error when used between two sets.
>
> SELECT 1 IS DISTINCT FROM SELECT 1;
> ERROR:  syntax error at or near "SELECT"
> LINE 1: SELECT 1 IS DISTINCT FROM SELECT 1;
>
> The example above could be written as:
>
> _Identical := (
> SELECT * FROM Foo WHERE FooID BETWEEN 1 AND 1
> IS NOT DISTINCT FROM
> SELECT * FROM Bar WHERE BarID BETWEEN 1 AND 1
> );
>
> Which would set _Identical to TRUE if the two sets are equal,
> and FALSE otherwise.
>
> Since it's currently a syntax error, there is no risk for changed
> behaviour for any existing executable queries.
>
> Thoughts?
>
> /Joel
>
>
> --
> 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] [COMMITTERS] pgsql: Release note updates.

[Alvaro Herrera]

Tom Lane wrote:
> Alvaro Herrera  writes:

> > May I suggest
> 
> > +  If CREATE INDEX CONCURRENTLY was used to build an index
> > +  that depends on a column not previously indexed, then rows
> > +  updated by transactions that ran concurrently with
> > +  the CREATE INDEX command could have missed receiving
> > +  index entries.
> 
> Can we say "pre-existing rows that were updated by...", or is that
> too optimistic?

Hmm.  Now that I think about it, it is probably possible to have a
transaction started before CIC that inserted a bunch of rows, and then
runs the UPDATE during the CIC race window.  Maybe there's a reason the
bug wouldn't hit in that case but I don't see it, and I'm not able to
test it right now to verify.

> (I fear this is too late for the current set of releases; I don't want
> to make the packagers redo their work just for this.  But we can correct
> it for future wraps.)

I think a large fraction of the readers will grab the release notes from
the website anyway, not their local copies.  And the "press release" is
a source that will get to a large number of readers too.  I think it's
fine not to re-wrap.

-- 
Álvaro Herrerahttps://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

[HACKERS] Press Release Draft - 2016-02-09 Cumulative Update

[Jonathan S. Katz]

Hi!

Below is the draft of the press release for the update this Thursday:

https://git.postgresql.org/gitweb/?p=press.git;a=blob;f=update_releases/current/20170209updaterelease.md;h=0cccb8986c08527f65f13d704a78c36bb8de7fef;hb=afc01091dea8a1597e8e21430edc3908c581ce0c
 


As there are a lot of updates I did my best to consolidate some of the bullet 
points and as usual, people are directed to the release notes.  Please let me 
know if there are any inaccuracies so I can fix them ASAP.

Thanks!

Jonathan

Re: [HACKERS] DROP SUBSCRIPTION and ROLLBACK

[Petr Jelinek]

On 07/02/17 16:26, Petr Jelinek wrote:
> On 07/02/17 13:10, Masahiko Sawada wrote:
>> I think the logical replication should not stop and the corresponding
>> replication slot and replication origin should not be removed until
>> the transaction commits.
>>
>> The solution for this I came up with is that the launcher process
>> stops the apply worker after DROP SUBSCRIPTION is committed rather
>> than DropSubscription does. And the apply worker drops replication
>> slot and replication origin before exits. Attached draft patch fixes
>> this issue.
>>
> 
> I don't think we can allow the slot drop to be postponed. There is too
> many failure scenarios where we would leave the remote slot in the
> database and that's not acceptable IMHO.
> 
> For example what happens if apply crashes during the DROP
> SUBSCRIPTION/COMMIT and is not started because the delete from catalog
> is now visible so the subscription is no longer there?
> 

Not to mention that slot creation/drop is not transactional by itself so
even if there was some way to tie remote transaction to local
transaction (like say 2pc), it would still not work with ROLLBACK.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, 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] [COMMITTERS] pgsql: Release note updates.

[Tom Lane]

Alvaro Herrera  writes:
> Tom Lane wrote:
>> (I fear this is too late for the current set of releases; I don't want
>> to make the packagers redo their work just for this.  But we can correct
>> it for future wraps.)

> I think a large fraction of the readers will grab the release notes from
> the website anyway, not their local copies.  And the "press release" is
> a source that will get to a large number of readers too.  I think it's
> fine not to re-wrap.

I was just wondering about that: I know the normal workflow for the
website versions of the manual is to build from the tagged release
points.  I intend to put the tags on the commits the official tarballs
correspond to, but maybe we can generate the website manuals from a
later commit this time?

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] DROP SUBSCRIPTION and ROLLBACK

[Petr Jelinek]

On 07/02/17 13:10, Masahiko Sawada wrote:
> Hi all,
> 
> While testing logical replciation I found that if the transaction
> issued DROP SUBSCRIPTION　rollbacks then the logical repliation stops
> and the subscription can never be removed later. The document says
> that the replication worker associated with the subscription will not
> stop until after the transaction that issued this command has
> committed but it doesn't work.

Ah then the docs are wrong and should be fixed. Maybe we should not
allow DROP SUBSCRIPTION inside transaction similarly to CREATE INDEX
CONCURRENTLY.

> The cause of this is that DropSubscription stops the apply worker and
> drops corresponding replication slot on publisher side without waiting
> for commit or rollback. The launcher process launches the apply worker
> again but the launched worker will fail to start logical replication
> because corresponding replication slot is already removed. And the
> orphan subscription can not be removed later.
> 
> I think the logical replication should not stop and the corresponding
> replication slot and replication origin should not be removed until
> the transaction commits.
> 
> The solution for this I came up with is that the launcher process
> stops the apply worker after DROP SUBSCRIPTION is committed rather
> than DropSubscription does. And the apply worker drops replication
> slot and replication origin before exits. Attached draft patch fixes
> this issue.
> 

I don't think we can allow the slot drop to be postponed. There is too
many failure scenarios where we would leave the remote slot in the
database and that's not acceptable IMHO.

For example what happens if apply crashes during the DROP
SUBSCRIPTION/COMMIT and is not started because the delete from catalog
is now visible so the subscription is no longer there?

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, 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] 'text' instead of 'unknown' in Postgres 10

[Daniele Varrazzo]

On Tue, Feb 7, 2017 at 2:59 PM, Andreas Karlsson  wrote:
> On 02/07/2017 03:14 PM, Daniele Varrazzo wrote:
>>
>> In psycopg '{}'::unknown is treated specially as an empty array and
>> converted into an empty list, which allows empty lists to be passed to
>> the server as arrays and returned back to python. Without the special
>> case, empty lists behave differently from non-empty ones. It seems
>> this behaviour cannot be maintained on PG 10 and instead users need to
>> specify some form of cast for their placeholder. Previously this would
>> have worked "as expected" and the 4th argument would have been an
>> empty list:
>>
>> cur.execute("SELECT %s, %s, %s, %s", (['x'], [42], [date(2017,1,1)],
>> [])); cur.fetchone()
>> (['x'], [42], [datetime.date(2017, 1, 1)], '{}')
>
>
> As Tom wrote this is the result of an intentional change, but no matter if
> that change is a good thing or not the above behavior sounds rather fragile.
> To me it does not seem safe to by default just assume that '{}' means the
> empty array, it might also have been intended to be the Python string "{}",
> the empty JSON object, or entirely something different.

Yes, it could be actually the case to drop it. The case for it is
quite thin anyway: if something comes from a query it will usually
have a type attached.

-- Daniele


-- 
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_restore is broken on 9.2 version.

[Stephen Frost]

* Stephen Frost (sfr...@snowman.net) wrote:
> Greetings,
> 
> * Rushabh Lathia (rushabh.lat...@gmail.com) wrote:
> > Commit c59a1a89035674c6efacc596d652528cebba37ec don't allow non-positive
> > number
> > of jobs. Now on 9.2 number of jobs get assigned to opts->number_of_jobs. If
> > user don't specify any value -j then default value will be always 0. Which
> > will
> > lead to the "invalid number of parallel jobs" error.
> > 
> > if (opts->number_of_jobs <= 0)
> > {
> > fprintf(stderr, _("%s: invalid number of parallel jobs\n"),
> > progname);
> > exit(1);
> > }
> > 
> > Please find attach patch to initialize default value for number of jobs to
> > 1.
> 
> Ugh.  This is what I get for thinking that our regression tests actually
> test even the basic things.
> 
> That fix should go into NewRestoreOptions() where the other not-zero
> settings go.
> 
> I'll fix it here in a few.

Fix pushed.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] 'text' instead of 'unknown' in Postgres 10

[Daniele Varrazzo]

On Tue, Feb 7, 2017 at 2:42 PM, Tom Lane  wrote:
> Daniele Varrazzo  writes:
>> testing with psycopg2 against Postgres 10 I've found a difference in
>> behaviour regarding literals, which are returned as text instead of
>> unknown. ...
>> Is this behaviour here to stay? Is there documentation for this change?
>
> Yup, see
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=1e7c4bb0049732ece651d993d03bb6772e5d281a
>
> The expectation is that clients will never see "unknown" output columns
> anymore.

Ok thank you, I'll document the change in behaviour.


> I don't have enough context to suggest a better definition for psycopg
> ... but maybe you could pay some attention to the Python type of the value
> you're handed?

In python the only type is the list, there is no specific "list of
integer" or such.


>> It seems
>> this behaviour cannot be maintained on PG 10 and instead users need to
>> specify some form of cast for their placeholder.
>
> Well, no version of PG has ever allowed this without a cast:
>
> regression=# select array[];
> ERROR:  cannot determine type of empty array
>
> so I'm not sure it's inconsistent for the same restriction to apply in the
> case you're describing.  I'm also unclear on why you are emphasizing the
> point of the array being empty, because '{1,2,3}'::unknown would have the
> same behavior.

The inconsistency is on our side: on python list [1,2,3] we generate
'ARRAY[1,2,3]', and empty lists are instead converted to '{}'
precisely because there is no such thing like unknown[] - nor we can
generate array[]::int[] because the Python list is empty and we don't
know if it would have contained integers or other stuff. Of course
this only works because we merge arguments in the adapter: moving to
use PQexecParams we couldn't allow that anymore and the user should be
uniformly concerned with adding casts to their queries (this is a
non-backward compatible change so only planned for a mythical psycopg3
version I've long desired to write but for which I have no resource).

Thank you for the clarification: I will assume the behaviour cannot be
maintained on PG 10 and think whether the treatment of '{}' is too
magical and drop it instead.


-- Daniele


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

[HACKERS] Idea on how to simplify comparing two sets

[Joel Jacobson]

 Hi hackers,

Currently there is no simple way to check if two sets are equal.

Looks like no RDBMS in the world has a simple command for it.

You have to do something like:

WITH
T1 AS (SELECT * FROM Foo WHERE FooID BETWEEN 1 AND 1),
T2 AS (SELECT * FROM Bar WHERE BarID BETWEEN 1 AND 1)
SELECT
GREATEST(
(SELECT COUNT(*) FROM T1),
(SELECT COUNT(*) FROM T2)
)
=
(SELECT COUNT(*) FROM (
SELECT * FROM T1
INTERSECT ALL
SELECT * FROM T2
) AS X)
INTO _Identical;

or,

SELECT 'Missmatch!' WHERE EXISTS (
SELECT * FROM Foo
FULL JOIN Bar ON (Foo.FooID = Bar.BarID AND
 Foo IS NOT DISTINCT FROM Bar)
WHERE TRUE
AND ( Foo.FooID BETWEEN 1 AND 1 AND
  Bar.BarID BETWEEN 1 AND 1)
AND ( Foo.FooID IS NULL OR
  Bar.BarID IS NULL);

Introducing new SQL keywords is of course not an option,
since it would possibly break backwards compatibility.

So here is an idea that doesn't break backwards compatibility:

Let's give a meaning for the existing IS DISTINCT and IS NOT DISTINCT,
that is currently a syntax error when used between two sets.

SELECT 1 IS DISTINCT FROM SELECT 1;
ERROR:  syntax error at or near "SELECT"
LINE 1: SELECT 1 IS DISTINCT FROM SELECT 1;

The example above could be written as:

_Identical := (
SELECT * FROM Foo WHERE FooID BETWEEN 1 AND 1
IS NOT DISTINCT FROM
SELECT * FROM Bar WHERE BarID BETWEEN 1 AND 1
);

Which would set _Identical to TRUE if the two sets are equal,
and FALSE otherwise.

Since it's currently a syntax error, there is no risk for changed
behaviour for any existing executable queries.

Thoughts?

/Joel


-- 
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] Cannot shutdown subscriber after DROP SUBSCRIPTION

[Fujii Masao]

On Tue, Feb 7, 2017 at 2:13 AM, Petr Jelinek
 wrote:
> On 06/02/17 17:33, Fujii Masao wrote:
>> On Sun, Feb 5, 2017 at 5:11 AM, Petr Jelinek
>>  wrote:
>>> On 03/02/17 19:38, Fujii Masao wrote:
 On Sat, Feb 4, 2017 at 12:49 AM, Fujii Masao  wrote:
> On Fri, Feb 3, 2017 at 10:56 AM, Kyotaro HORIGUCHI
>  wrote:
>> At Fri, 3 Feb 2017 01:02:47 +0900, Fujii Masao  
>> wrote in 
>> 

Re: [HACKERS] 'text' instead of 'unknown' in Postgres 10

[Andreas Karlsson]

On 02/07/2017 03:14 PM, Daniele Varrazzo wrote:

In psycopg '{}'::unknown is treated specially as an empty array and
converted into an empty list, which allows empty lists to be passed to
the server as arrays and returned back to python. Without the special
case, empty lists behave differently from non-empty ones. It seems
this behaviour cannot be maintained on PG 10 and instead users need to
specify some form of cast for their placeholder. Previously this would
have worked "as expected" and the 4th argument would have been an
empty list:

cur.execute("SELECT %s, %s, %s, %s", (['x'], [42], [date(2017,1,1)],
[])); cur.fetchone()
(['x'], [42], [datetime.date(2017, 1, 1)], '{}')


As Tom wrote this is the result of an intentional change, but no matter 
if that change is a good thing or not the above behavior sounds rather 
fragile. To me it does not seem safe to by default just assume that '{}' 
means the empty array, it might also have been intended to be the Python 
string "{}", the empty JSON object, or entirely something different.


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] [PATCH] configure-time knob to set default ssl ciphers

[Pavel Raiskup]

Hi hackers,

in Fedora, there's crypto initiative where people try to consolidate ssl
cipher settings for (majority of) Fedora services (PostgreSQL is
included).

PostgreSQL server uses 'HIGH:MEDIUM:+3DES:!aNULL' cipher set by default,
but what Fedora would like to have is 'PROFILE=SYSTEM' (works with
Fedora-patched OpenSSL, so please don't waste your time with checking this
elsewhere).  What that really does is:

  kEECDH:kRSA:kEDH:kPSK:kDHEPSK:kECDHEPSK:!EXP:!DES:!RC4:!RC2:!IDEA\
  :!SEED:!eNULL:!aNULL:!MD5:!SSLv2

.. but that's just for the record (should be subset of upstream default);
more info in RH bug [1].

I'd like to propose the attached patch, so we could (without downstream
patching) do
$ ./configure ... --with-openssl-be-ciphers=PROFILE=SYSTEM

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1348125

Thanks for considering!
Pavel
>From dae9b8c0345b65882c221a4062f435cf657fe55a Mon Sep 17 00:00:00 2001
From: Pavel Raiskup 
Date: Wed, 18 Jan 2017 13:34:55 +0100
Subject: [PATCH] Allow setting distribution-specific cipher set

Fedora OpenSSL maintainers invented a way to specify consolidated,
per-system cipher set [1] and it is our packaging policy to comply
(if this is a bit meaningful).

So for such situations ./configure options comes in handy instead
of downstream-patching, per Red Hat bug report [2].

[1] https://fedoraproject.org/wiki/Packaging:CryptoPolicies
[2] https://bugzilla.redhat.com/show_bug.cgi?id=1348125
---
 configure| 32 
 configure.in |  8 
 src/backend/utils/misc/guc.c |  4 
 src/include/pg_config.h.in   |  3 +++
 4 files changed, 47 insertions(+)

diff --git a/configure b/configure
new file mode 100755
index bb285e4..9e7fa82
*** a/configure
--- b/configure
*** with_bsd_auth
*** 831,836 
--- 831,837 
  with_ldap
  with_bonjour
  with_openssl
+ with_openssl_be_ciphers
  with_selinux
  with_systemd
  with_readline
*** Optional Packages:
*** 1521,1526 
--- 1522,1529 
--with-ldap build with LDAP support
--with-bonjour  build with Bonjour support
--with-openssl  build with OpenSSL support
+   --with-openssl-be-ciphers=STRING
+   Replace the default list of server-supported ciphers
--with-selinux  build with SELinux support
--with-systemd  build with systemd support
--without-readline  do not use GNU Readline nor BSD Libedit for editing
*** fi
*** 5712,5717 
--- 5715,5749 
  $as_echo "$with_openssl" >&6; }
  
  
+ 
+ 
+ 
+ # Check whether --with-openssl-be-ciphers was given.
+ if test "${with_openssl_be_ciphers+set}" = set; then :
+   withval=$with_openssl_be_ciphers;
+   case $withval in
+ yes)
+   as_fn_error $? "argument required for --with-openssl-be-ciphers option" "$LINENO" 5
+   ;;
+ no)
+   as_fn_error $? "argument required for --with-openssl-be-ciphers option" "$LINENO" 5
+   ;;
+ *)
+ 
+ cat >>confdefs.h <<_ACEOF
+ #define PG_DEFAULT_SSL_CIPHERS "$with_openssl_be_ciphers"
+ _ACEOF
+ 
+   ;;
+   esac
+ 
+ fi
+ 
+ 
+ 
+ { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether to replace default OpenSSL cypher set" >&5
+ $as_echo_n "checking whether to replace default OpenSSL cypher set... " >&6; }
+ 
  #
  # SELinux
  #
diff --git a/configure.in b/configure.in
new file mode 100644
index 09a887d..f26f1fa
*** a/configure.in
--- b/configure.in
*** PGAC_ARG_BOOL(with, openssl, no, [build
*** 712,717 
--- 712,725 
  AC_MSG_RESULT([$with_openssl])
  AC_SUBST(with_openssl)
  
+ PGAC_ARG_REQ(with, openssl-be-ciphers, [STRING],
+  [Replace the default list of server-supported ciphers],
+  [AC_DEFINE_UNQUOTED([PG_DEFAULT_SSL_CIPHERS],
+  ["$with_openssl_be_ciphers"],
+  [Re-define the default for server ssl_ciphers option])])
+ 
+ AC_MSG_CHECKING([whether to replace default OpenSSL cypher set])
+ 
  #
  # SELinux
  #
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
new file mode 100644
index 4f1891f..8b4e576
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*** static struct config_string ConfigureNam
*** 3508,3514 
--- 3508,3518 
  		},
  		,
  #ifdef USE_SSL
+ #ifdef PG_DEFAULT_SSL_CIPHERS
+ 		PG_DEFAULT_SSL_CIPHERS,
+ #else
  		"HIGH:MEDIUM:+3DES:!aNULL",
+ #endif
  #else
  		"none",
  #endif
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
new file mode 100644
index 7dbfa90..8367744
*** a/src/include/pg_config.h.in
--- b/src/include/pg_config.h.in
***
*** 738,743 
--- 738,746 
  /* Define to the version of this package. */
  #undef PACKAGE_VERSION
  
+ /* Re-define the default for server ssl_ciphers option */
+ #undef PG_DEFAULT_SSL_CIPHERS
+ 
  /* Define to the name of a signed 128-bit 

Re: [HACKERS] DROP SUBSCRIPTION and ROLLBACK

[Fujii Masao]

On Tue, Feb 7, 2017 at 9:10 PM, Masahiko Sawada  wrote:
> Hi all,
>
> While testing logical replciation I found that if the transaction
> issued DROP SUBSCRIPTION　rollbacks then the logical repliation stops
> and the subscription can never be removed later. The document says
> that the replication worker associated with the subscription will not
> stop until after the transaction that issued this command has
> committed but it doesn't work.

Yeah, this is a bug.

ISTM that CREATE SUBSCRIPTION also has the similar issue. It creates
the replication slot on the publisher side before the transaction has been
committed. Even if the transaction is rollbacked, that replication slot is
not removed. That is, in a transaction block, we should not connect to
the publisher. Instead, the launcher or worker should do.

> The cause of this is that DropSubscription stops the apply worker and
> drops corresponding replication slot on publisher side without waiting
> for commit or rollback. The launcher process launches the apply worker
> again but the launched worker will fail to start logical replication
> because corresponding replication slot is already removed. And the
> orphan subscription can not be removed later.
>
> I think the logical replication should not stop and the corresponding
> replication slot and replication origin should not be removed until
> the transaction commits.

Yes.

> The solution for this I came up with is that the launcher process
> stops the apply worker after DROP SUBSCRIPTION is committed rather
> than DropSubscription does. And the apply worker drops replication
> slot and replication origin before exits. Attached draft patch fixes
> this issue.
>
> Please give me feedback.

The patch failed to apply to HEAD.

+ worker = logicalrep_worker_find(subid);
+ if (worker)
  {
- heap_close(rel, NoLock);
- return;
+ if (stmt->drop_slot)
+ worker->drop_slot = true;
+ worker->need_to_stop = true;

"drop_slot" and "need_to_stop" seem to be set to true even if the transaction
is rollbacked. This would cause the same problem that you're trying to fix.

I think that we should make the launcher periodically checks pg_subscription
and stop the worker if there is no its corresponding subscription. Then,
if necessary, the worker should remove its replication slot from the publisher.

Regards,

-- 
Fujii Masao


-- 
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_restore is broken on 9.2 version.

[Stephen Frost]

Greetings,

* Rushabh Lathia (rushabh.lat...@gmail.com) wrote:
> Commit c59a1a89035674c6efacc596d652528cebba37ec don't allow non-positive
> number
> of jobs. Now on 9.2 number of jobs get assigned to opts->number_of_jobs. If
> user don't specify any value -j then default value will be always 0. Which
> will
> lead to the "invalid number of parallel jobs" error.
> 
> if (opts->number_of_jobs <= 0)
> {
> fprintf(stderr, _("%s: invalid number of parallel jobs\n"),
> progname);
> exit(1);
> }
> 
> Please find attach patch to initialize default value for number of jobs to
> 1.

Ugh.  This is what I get for thinking that our regression tests actually
test even the basic things.

That fix should go into NewRestoreOptions() where the other not-zero
settings go.

I'll fix it here in a few.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] 'text' instead of 'unknown' in Postgres 10

[Tom Lane]

Daniele Varrazzo  writes:
> testing with psycopg2 against Postgres 10 I've found a difference in
> behaviour regarding literals, which are returned as text instead of
> unknown. ...
> Is this behaviour here to stay? Is there documentation for this change?

Yup, see
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=1e7c4bb0049732ece651d993d03bb6772e5d281a

The expectation is that clients will never see "unknown" output columns
anymore.

> In psycopg '{}'::unknown is treated specially as an empty array and
> converted into an empty list, which allows empty lists to be passed to
> the server as arrays and returned back to python. Without the special
> case, empty lists behave differently from non-empty ones.

I think you need to rethink that anyway, because in the old code,
whether such a value came back as text or unknown was dependent on
context, for example

regression=# select pg_typeof(x) from (select '' as x) ss;
 pg_typeof 
---
 unknown
(1 row)

regression=# select pg_typeof(x) from (select distinct '' as x) ss;
 pg_typeof 
---
 text
(1 row)

HEAD yields "text" for both of those cases, which seems a much saner
behavior to me.

I don't have enough context to suggest a better definition for psycopg
... but maybe you could pay some attention to the Python type of the value
you're handed?

> It seems
> this behaviour cannot be maintained on PG 10 and instead users need to
> specify some form of cast for their placeholder.

Well, no version of PG has ever allowed this without a cast:

regression=# select array[];
ERROR:  cannot determine type of empty array

so I'm not sure it's inconsistent for the same restriction to apply in the
case you're describing.  I'm also unclear on why you are emphasizing the
point of the array being empty, because '{1,2,3}'::unknown would have the
same behavior.

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] 'text' instead of 'unknown' in Postgres 10

[Pavel Stehule]

Hi

2017-02-07 15:14 GMT+01:00 Daniele Varrazzo :

> Hello,
>
> testing with psycopg2 against Postgres 10 I've found a difference in
> behaviour regarding literals, which are returned as text instead of
> unknown. In previous versions:
>
> In [2]: cnn = psycopg2.connect('')
> In [3]: cur = cnn.cursor()
> In [7]: cur.execute("select 'x'")
> In [9]: cur.description[0][1]
> Out[9]: 705
>
> In pg10 master:
>
> In [10]: cnn = psycopg2.connect('dbname=postgres host=localhost
> port=54310')
> In [11]: cur = cnn.cursor()
> In [12]: cur.execute("select 'x'")
> In [13]: cur.description[0][1]
> Out[13]: 25
>
> what is somewhat surprising is that unknown seems promoted to text "on
> the way out" from a query; in previous versions both columns of this
> query would have been "unknown".
>
> postgres=# select pg_typeof('x'), pg_typeof(foo) from (select 'x' as foo)
> x;
>  pg_typeof | pg_typeof
> ---+---
>  unknown   | text
>
> Is this behaviour here to stay? Is there documentation for this change?
>
> In psycopg '{}'::unknown is treated specially as an empty array and
> converted into an empty list, which allows empty lists to be passed to
> the server as arrays and returned back to python. Without the special
> case, empty lists behave differently from non-empty ones. It seems
> this behaviour cannot be maintained on PG 10 and instead users need to
> specify some form of cast for their placeholder. Previously this would
> have worked "as expected" and the 4th argument would have been an
> empty list:
>
> cur.execute("SELECT %s, %s, %s, %s", (['x'], [42], [date(2017,1,1)],
> [])); cur.fetchone()
> (['x'], [42], [datetime.date(2017, 1, 1)], '{}')
>
> Should I just take this test off from the test suite and document the
> adapter as behaving differently on PG 10?
>
> Thank you very much
>

I see similar issue in plpgsql_check

create function test_t(OUT t) returns t AS $$
begin
$1 := null;
end;
$$ language plpgsql;

Now the "null" is text type implicitly ("unknown" was before)

select * from plpgsql_check_function('test_t()', performance_warnings :=
true);

plpgsql_check_function
--
 warning:42804:3:assignment:target type is different type than source type
 Detail: cast "text" value to "integer" type
 Hint: The input expression type does not have an assignment cast to the
target type.
(3 rows)

It is a regression from my view - unknown had more sense in this case.

Regards

Pavel




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

[HACKERS] pg_restore is broken on 9.2 version.

[Rushabh Lathia]

Hi All,

Commit c59a1a89035674c6efacc596d652528cebba37ec don't allow non-positive
number
of jobs. Now on 9.2 number of jobs get assigned to opts->number_of_jobs. If
user don't specify any value -j then default value will be always 0. Which
will
lead to the "invalid number of parallel jobs" error.

if (opts->number_of_jobs <= 0)
{
fprintf(stderr, _("%s: invalid number of parallel jobs\n"),
progname);
exit(1);
}

Please find attach patch to initialize default value for number of jobs to
1.

Thanks,
Rushabh Lathia
www.EnterpriseDB.com


pg_restore_fix.patch
Description: binary/octet-stream

-- 
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] GSoC 2017

[Stephen Frost]

Ruben,

* Ruben Buchatskiy (ru...@ispras.ru) wrote:
> Difficulty Level
> Moderate-level; however, microoptimizations might be hard.
> Probably it will also be hard to keep the whole architecture as clean as it is
> now.

The above difficulty level looks fine, but doesn't match what's on the
wiki.  What's on the wiki looks like a copy/paste from one of the
SSI-related items.

Please fix.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] [COMMITTERS] pgsql: Release note updates.

[Tom Lane]

Alvaro Herrera  writes:
> Tom Lane wrote:
>> Release note updates.

> Sorry for not noticing earlier, but there is a bug in the notes:

Mmm, right.

> May I suggest

> +  If CREATE INDEX CONCURRENTLY was used to build an index
> +  that depends on a column not previously indexed, then rows
> +  updated by transactions that ran concurrently with
> +  the CREATE INDEX command could have missed receiving
> +  index entries.

Can we say "pre-existing rows that were updated by...", or is that
too optimistic?

(I fear this is too late for the current set of releases; I don't want
to make the packagers redo their work just for this.  But we can correct
it for future wraps.)

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] 'text' instead of 'unknown' in Postgres 10

[Daniele Varrazzo]

Hello,

testing with psycopg2 against Postgres 10 I've found a difference in
behaviour regarding literals, which are returned as text instead of
unknown. In previous versions:

In [2]: cnn = psycopg2.connect('')
In [3]: cur = cnn.cursor()
In [7]: cur.execute("select 'x'")
In [9]: cur.description[0][1]
Out[9]: 705

In pg10 master:

In [10]: cnn = psycopg2.connect('dbname=postgres host=localhost port=54310')
In [11]: cur = cnn.cursor()
In [12]: cur.execute("select 'x'")
In [13]: cur.description[0][1]
Out[13]: 25

what is somewhat surprising is that unknown seems promoted to text "on
the way out" from a query; in previous versions both columns of this
query would have been "unknown".

postgres=# select pg_typeof('x'), pg_typeof(foo) from (select 'x' as foo) x;
 pg_typeof | pg_typeof
---+---
 unknown   | text

Is this behaviour here to stay? Is there documentation for this change?

In psycopg '{}'::unknown is treated specially as an empty array and
converted into an empty list, which allows empty lists to be passed to
the server as arrays and returned back to python. Without the special
case, empty lists behave differently from non-empty ones. It seems
this behaviour cannot be maintained on PG 10 and instead users need to
specify some form of cast for their placeholder. Previously this would
have worked "as expected" and the 4th argument would have been an
empty list:

cur.execute("SELECT %s, %s, %s, %s", (['x'], [42], [date(2017,1,1)],
[])); cur.fetchone()
(['x'], [42], [datetime.date(2017, 1, 1)], '{}')

Should I just take this test off from the test suite and document the
adapter as behaving differently on PG 10?

Thank you very much

-- Daniele


-- 
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] GSoC 2017 - LAST CALL

[Stephen Frost]

Amit,

* Amit Kapila (amit.kapil...@gmail.com) wrote:
> On Tue, Feb 7, 2017 at 7:09 PM, Stephen Frost  wrote:
> > If you have any ideas for projects, please go to:
> >
> > https://wiki.postgresql.org/wiki/GSoC_2017
> 
> There are few entries which seem to be redundant:
> 
> > Implement WAL logging for hash indexes

Apologies, to be clear, the things listed under "Core Project Ideas" are
*not* going to be included in our submission, only those project ideas
listed below in the structured format will be.

I'm going to remove those "Core Project Ideas" from the page- they're
still there on the 2016 page, if people want to pull from them to
provide properly described project ideas.

> > Invent an async interface for the Append() node to use, allowing a way to 
> > parallelize queries across multiple remote FDWs or local tablespaces.

Again, that's under the "core project ideas" section, which was just
pulled forward from 2016 and are not going to be included.

If you have any comments on the properly formatted/described project
ideas, that would be great.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] GSoC 2017 - LAST CALL

[Amit Kapila]

On Tue, Feb 7, 2017 at 7:09 PM, Stephen Frost  wrote:
> All,
>
> This is the LAST CALL for applications to GSoC 2017 for PostgreSQL.
>
> If you have any ideas for projects, please go to:
>
> https://wiki.postgresql.org/wiki/GSoC_2017
>

There are few entries which seem to be redundant:

> Implement WAL logging for hash indexes

We already have a patch for this feature.

> Invent an async interface for the Append() node to use, allowing a way to 
> parallelize queries across multiple remote FDWs or local tablespaces.

Isn't this same what we are discussing in thread:
https://www.postgresql.org/message-id/CA%2BTgmoaXQEt4tZ03FtQhnzeDEMzBck%2BLrni0UWHVVgOTnA6C1w%40mail.gmail.com

-- 
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] LWLock optimization for multicore Power machines

[Alexander Korotkov]

On Tue, Feb 7, 2017 at 3:16 PM, Bernd Helmle  wrote:

> Am Montag, den 06.02.2017, 22:44 +0300 schrieb Alexander Korotkov:
>
>2. Also could you run each test longer: 3-5 mins, and run them with
>variety of clients count?
>
>
> So here are some other results. I've changed max_connections to 300. The
> bench was prewarmed and run 300s each.
> I could run more benches, if necessary.
>

Thank you very much for benchmarks!

There is clear win of both lwlock-power-1.patch and lwlock-power-3.patch in
comparison to master.  Difference between lwlock-power-1.patch and
lwlock-power-3.patch seems to be within the margin of error.  But win isn't
as high as I observed earlier.  And I wonder why absolute numbers are lower
than in our earlier experiments.  We used IBM E880 which is actually two
nodes with interconnect.  However interconnect is not fast enough to make
one PostgreSQL instance work on both nodes.  Thus, used half of IBM E880
which has 4 sockets and 32 physical cores.  While you use IBM E850 which is
really single node with 4 sockets and 48 physical cores.  Thus, it seems
that you have lower absolute numbers on more powerful hardware.  That makes
me uneasy and I think we probably don't get the best from this hardware.
Just in case, do you use SMT=8?

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

Re: [HACKERS] GSoC 2017

[Stephen Frost]

* Stephen Frost (sfr...@snowman.net) wrote:
> * Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
> > On 2017/02/06 20:51, Ruben Buchatskiy wrote:
> > > Also we have seen in the mailing list that Kumar Rajeev had been
> > > investigating this idea too, and he reported that the results were
> > > impressive (unfortunately, without specifying more details):
> > > 
> > > https://www.postgresql.org/message-id/BF2827DCCE55594C8D7A8F7FFD3AB77159A9B904%40szxeml521-mbs.china.huawei.com
> > 
> > You might also want to take a look at some of the ongoing work in this area:
> > 
> > WIP: Faster Expression Processing and Tuple Deforming (including JIT)
> > https://www.postgresql.org/message-id/flat/20161206034955.bh33paeralxbtluv%40alap3.anarazel.de
> 
> Yes, exactly that.  Please review what's been currently done and,
> ideally, have someone like Andres comment on your plan.
> 
> Perhaps you could arrange something with him as the mentor, since it
> looked like you didn't have any specific mentors listed in a quick look.
> That's definitely something that will be needed to include this project.

Apologies, looks like you do have a couple of mentors listed on the
wiki, so that looks good.

Thanks!

Stephen


signature.asc
Description: Digital signature

[HACKERS] GSoC 2017 - LAST CALL

[Stephen Frost]

All,

This is the LAST CALL for applications to GSoC 2017 for PostgreSQL.

If you have any ideas for projects, please go to:

https://wiki.postgresql.org/wiki/GSoC_2017

and add them, using the format listed there.

I will be submitting our application to Google tomorrow morning (the
deadline is Feb 9th, 1600 UTC, but I'd like to do it a bit early to
have time to address any issues).  Please get your project ideas
submitted so we can include them.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] pageinspect: Hash index support

[Robert Haas]

On Sat, Feb 4, 2017 at 7:06 AM, Ashutosh Sharma  wrote:
>> As far as I can tell, the hash_bitmap_info() function is doing
>> something completely ridiculous.  One would expect that the purpose of
>> this function was to tell you about the status of pages in the bitmap.
>> The documentation claims that this is what the function does: it
>> claims that this function "shows the status of a bit in the bitmap
>> page for a particular overflow page".  So you would think that what
>> the function would do is:
>>
>> 1. Work out which bitmap page contains the bit for the page number in 
>> question.
>> 2. Read that bitmap page.
>> 3. Indicate the status of that bit within that page.
>>
>> However, that's not what the function actually does.  Instead, it does this:
>>
>> 1. Go examine the overflow page and see whether hasho_prevblkno.  If
>> so, claim that the bit is set in the bitmap; if not, claim that it
>> isn't.
>> 2. Work out which bitmap page contains the bit for the page number in 
>> question.
>> 3. But don't look at it.  Instead, tell the user which bitmap page and
>> bit you would have looked at, but instead of returning the status of
>> that bit, return the value you computed in step 1.
>>
>> I do not think this can be the right approach.
>
> Yes, It is not a right approach. As I mentioned in [1], the overflow
> page being freed is completely filled with zero values which means it
> is not in a readable state. So, we won't be able to examine a free
> overflow page. Considering these facts, I would take following
> approach,
>
> 1) Check if an overflow page is a new page. If so, read a bitmap page
> to confirm if a bit corresponding to this overflow page is clear or
> not. For this, I would first add Assert statement to ensure that the
> bit is clear and if it is, then set the statusbit as false indicating
> that the page is free.
>
> 2) In case if an overflow page is not new, first verify if it is
> really an overflow page and if so, check if the bit corresponding to
> it in the bitmap page is SET. If so, set the statusbit as true; If
> not, we would see an assertion failure happening.

I think this is complicated and not what anybody wants.  I think you
should do exactly what I said above: return true if the bit is set in
the bitmap, and false if it isn't.  Full stop.  Don't read or do
anything with the underlying page.  Only read the bitmap page.

-- 
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] GSoC 2017

[Stephen Frost]

Greetings,

* Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
> On 2017/02/06 20:51, Ruben Buchatskiy wrote:
> > Also we have seen in the mailing list that Kumar Rajeev had been
> > investigating this idea too, and he reported that the results were
> > impressive (unfortunately, without specifying more details):
> > 
> > https://www.postgresql.org/message-id/BF2827DCCE55594C8D7A8F7FFD3AB77159A9B904%40szxeml521-mbs.china.huawei.com
> 
> You might also want to take a look at some of the ongoing work in this area:
> 
> WIP: Faster Expression Processing and Tuple Deforming (including JIT)
> https://www.postgresql.org/message-id/flat/20161206034955.bh33paeralxbtluv%40alap3.anarazel.de

Yes, exactly that.  Please review what's been currently done and,
ideally, have someone like Andres comment on your plan.

Perhaps you could arrange something with him as the mentor, since it
looked like you didn't have any specific mentors listed in a quick look.
That's definitely something that will be needed to include this project.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] 0/NULL/NIL assignments in build_*_rel()

[Robert Haas]

On Mon, Feb 6, 2017 at 12:04 AM, Tom Lane  wrote:
> Ashutosh Bapat  writes:
>> Both build_simple_rel() and build_join_rel() allocate RelOptInfo using
>> makeNode(), which returned a zeroed out memory. The functions then
>> assign values like false, NULL, 0 or NIL which essentially contain
>> zero valued bytes. This looks like needless work. So, we are spending
>> some CPU cycles unnecessarily in those assignments. That may not be
>> much time wasted, but whenever someone adds a field to RelOptInfo,
>> those functions need to be updated with possibly a zero value
>> assignment. That looks like an unnecessary maintenance burden. Should
>> we just drop all those zero value assignments from there?
>
> I'd vote for not.  The general programming style in the backend is to
> ignore the fact that makeNode() zeroes the node's storage and initialize
> all the fields explicitly.

I know that's your preference so I try not to spend too much time
arguing about it but personally I don't like it.  If I want to find
the places where a particular structure member gets set to a value,
the places where it's getting set to NULL aren't interesting, because
I have to think about that case anyway; somebody might have inserted a
makeNode() call without explicit initializations someplace; people do
that sometimes.  So for me, the places where we reinitialize storage
that is already-zeroed seems like a waste not only of cycles but of
brainpower.  However, as I say, I recognize that we see the world
differently on this point.

-- 
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] WAL consistency check facility

[Robert Haas]

On Tue, Feb 7, 2017 at 6:32 AM, Amit Kapila  wrote:
> On Tue, Jan 31, 2017 at 9:36 PM, Robert Haas  wrote:
>>
>> +if (!HeapTupleHeaderXminFrozen(page_htup))
>> +page_htup->t_infomask |= HEAP_XACT_MASK;
>> +else
>> +page_htup->t_infomask |= HEAP_XMAX_COMMITTED |
>> HEAP_XMAX_INVALID;
>>
>> Comment doesn't address this logic.  Also, the "else" case shouldn't
>> exist at all, I think.
>>
>
> In the *if* check, it just checks frozen status of xmin, so I think
> you need to mask xmax related bits in else check.  Can you explain
> what makes you think that the else case shouldn't exist?

Oh, you're right.

-- 
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 Auto-Prewarm.

[Beena Emerson]

Hello,

On Tue, Feb 7, 2017 at 5:14 PM, Mithun Cy 
wrote:

> On Tue, Feb 7, 2017 at 12:24 PM, Amit Kapila 
> wrote:
> > On Tue, Feb 7, 2017 at 11:53 AM, Beena Emerson 
> wrote:
> >> Are 2 workers required?
> >>
> >
> > I think in the new design there is a provision of launching the worker
> > dynamically to dump the buffers, so there seems to be a need of
> > separate workers for loading and dumping the buffers.  However, there
> > is no explanation in the patch or otherwise when and why this needs a
> > pair of workers.  Also, if the dump interval is greater than zero,
> > then do we really need to separately register a dynamic worker?
>
> We have introduced a new value  -1 for pg_prewarm.dump_interval this
> means we will not dump at all, At this state, I thought auto
> pg_prewarm process need not run at all, so I coded to exit the auto
> pg_prewarm immediately. But If the user decides to start the auto
> pg_prewarm to dump only without restarting the server, I have
> introduced a launcher function "launch_pg_prewarm_dump" to restart the
> auto pg_prewarm only to dump. Since now we can launch worker only to
> dump, I thought we can redistribute the code between two workers, one
> which only does prewarm (load only) and another dumps periodically.
> This helped me to modularize and reuse the code. So once load worker
> has finished its job, it registers a dump worker and then exists.
> But if max_worker_processes is not enough to launch the "auto
> pg_prewarm dump" bgworker
> We throw an error
> 2017-02-07 14:51:59.789 IST [50481] ERROR:  registering dynamic
> bgworker "auto pg_prewarm dump" failed c
> 2017-02-07 14:51:59.789 IST [50481] HINT:  Consider increasing
> configuration parameter "max_worker_processes".
>
> Now thinking again instead of such error and then correcting same by
> explicitly launching the auto pg_prewarm dump bgwroker through
> launch_pg_prewarm_dump(), I can go back to original design where there
> will be one worker which loads and then dumps periodically. And
> launch_pg_prewarm_dump will relaunch dump only activity of that
> worker. Does this sound good?
>

 Yes it would be better to have only one pg_prewarm worker as the loader is
idle for the entire server run time after the initial load activity of few
secs.

-- 
Thank you,

Beena Emerson

Have a Great Day!

Re: [HACKERS] Proposal : For Auto-Prewarm.

[Beena Emerson]

Hello,

On Tue, Feb 7, 2017 at 5:52 PM, Mithun Cy 
wrote:

> Thanks Beena,
>
> On Tue, Feb 7, 2017 at 4:46 PM, Beena Emerson 
> wrote:
> > Few more comments:
> >
> > = Background worker messages:
> >
> > - Workers when launched, show messages like: "logical replication
> launcher
> > started”, "autovacuum launcher started”. We should probably have a
> similar
> > message to show that the pg_prewarm load and dump bgworker has started.
>
> -- Thanks, I will add startup and shutdown message.
>
> > - "auto pg_prewarm load: number of buffers to load x”, other messages
> show
> > space before and after “:”, we should keep it consistent through out.
> >
>
> -- I think you are testing patch 03. The latest patch_04 have
> corrected same. Can you please re-test it.
>

I had initially written comments for 03 and then again tested for 04 and
retained comments valid for it. I guess I missed to removed this. Sorry.


>
> >
> > = Action of -1.
> > I thought we decided that interval value of -1 would mean that the auto
> > prewarm worker will not be run at all. With current code, -1 is
> explained to
> > mean it will not dump. I noticed that reloading with new option as -1
> stops
> > both the workers but restarting loads the data and then quits. Why does
> it
> > allow loading with -1? Please explain this better in the documents.
> >
>
> -- '-1' means we do not want to dump at all. So we decide not to
> continue with launched bgworker and it exits. As per your first
> comment, if I register the startup and shutdown messages for auto
> pg_prewarm I think it will look better. Will try to explain it in a
> better way in documentation. The "auto pg_prewarm load" will not be
> affected with dump_interval value. It will always start, load(prewarm)
> and then exit.
>
> >
> > = launch_pg_prewarm_dump()
>
> > =# SELECT launch_pg_prewarm_dump();
> >  launch_pg_prewarm_dump
> > 
> >   53552
> > (1 row)
> >
> > $ ps -ef | grep 53552
> > b_emers+  53555   4391  0 16:21 pts/100:00:00 grep --color=auto 53552
>
> -- If dump_interval = -1 "auto pg_prewarm dump" will exit immediately.
>
> > = Function names
> > - load_now could be better named as load_file, load_dumpfile or similar.
> > - dump_now -> dump_buffer or  better?
>
> I did choose load_now and dump_now to indicate we are doing it
> immediately as invoking them was based on a timer/state. Probably we
> can improve that but dump_buffer, load_file may not be the right
> replacement.
>
> >
> > = Corrupt file
> > if the dump file is corrupted, the system crashes and the prewarm
> bgworkers
> > are not restarted. This needs to be handled better.
> >
> > WARNING:  terminating connection because of crash of another server
> process
> > 2017-02-07 16:36:58.680 IST [54252] DETAIL:  The postmaster has commanded
> > this server process to roll back the current transaction and exit,
> because
> > another server process exited abnormally and possibly corrupted shared
> > memory
>
> --- Can you please paste you autopgprewarm.save file, I edited the
> file manually to some illegal entry but did not see any crash.  Only
> we failed to load as block number were invalid. Please share your
> tests so that I can reproduce same.
>

I only changed the fork number from 0 to 10 in one of the entry.


> > = Documentation
> >
> > I feel the documentation needs to be improved greatly.
> >
> > - The first para in pg_prewarm should mention the autoload feature too.
> >
> > - The new section should be named “The pg_prewarm autoload” or something
> > better. "auto pg_prewarm bgworker” does not seem appropriate.  The
> > configuration parameter should also be listed out clearly like in
> auth-delay
> > page. The new function launch_pg_prewarm_dump() should be listed under
> > Functions.
>
> -- Thanks I will try to improve the documentation. And, put more details
> there.
>
>

--
Thank you,

Beena Emerson

Have a Great Day!