Re: [HACKERS] [PATCH] Logical decoding timeline following take II

[Craig Ringer]

It's unlikely this will get in as a standalone patch, so I'm closing
the CF entry for it as RwF

https://commitfest.postgresql.org/11/779/

It is now being tracked as part of logical decoding on standby at

https://commitfest.postgresql.org/12/788/

in thread "Logical decoding on standby", which began as
https://www.postgresql.org/message-id/flat/CAMsr+YFi-LV7S8ehnwUiZnb=1h_14PwQ25d-vyUNq-f5S5r=z...@mail.gmail.com#CAMsr+YFi-LV7S8ehnwUiZnb=1h_14PwQ25d-vyUNq-f5S5r=z...@mail.gmail.com
.

-- 
 Craig Ringer   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] PSQL commands: \quit_if, \quit_unless

[Fabien COELHO]

Hello,


I think it's really time we seriously considered adding some flow
control logic, though.


Yeah, maybe.  I'd be interested to see a fully worked out proposal
for that.


I agree that designing a fuller proposal before including individual parts 
would be great and result in a more consistent result.


In order to bootstrap the discussion, I suggest the following:

 - boolexpr is a simple "boolean" (t, non 0 int, non empty string.. as
   proposed by Corey and Pavel) or !/not boolexp ; it could be extended if
   necessary, but I would try to avoid that, as

 - actual more complex expressions could be left to the server through SQL
   which simplifies the client a lot by avoiding an expression language
   altogether

 - then having a conditional block is very versatile and can be adapted to
   many use cases... maybe all

 - \quit CODE, or I would prefer \exit CODE, could be used to exit while
   controlling the status

It could look like (although I do not like gset in this context, but 
anyway):


 SELECT ... AS has_foo_extension \gset
 SELECT ... AS has_bla_extension \gset
 \if :has_foo_extension
   ...
 \elif :has_bla_extension
   ...
 \else -- no foo nor bla extension
   \echo please install foo or bla extension
   \exit 1
 \fi -- extension
 ...
 SELECT ... AS has_xxx_feature \gset
 \if ! :has_xxx_feature
  \echo "feature xxx is needed, aborting"
  \exit 2
 \fi
 ...

--
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] GIN non-intrusive vacuum of posting tree

[Vladimir Borodin]

> 28 нояб. 2016 г., в 20:31, Andrew Borodin  написал(а):
> 
> This patch solved a problem encountered by Evgeniy Efimkin and
> Vladimir Borodin from Yandex.Mail.
> 
> and eventually deleting some of data. This testbed showed VACUUM
> holding inserts for up to tenths of seconds. They claim that proposed
> patch made vacuum invisible in this test.
> 
> Evgeniy, Vladimir, if I missed something or you have something to add,
> please join discussion.

Yep, in our load environment the table is not so big (~ 50 GB), GIN index size 
is ~ 1 GB. And under our load profile we have seen 90 seconds of impossibility 
to do an insert into the table because of vacuuming this index. I confirm that 
with this patch we now don’t see any spikes of errors as it was previously.

--
May the force be with you…
https://simply.name

Re: [HACKERS] Tackling JsonPath support

[Pavel Stehule]

2016-11-29 7:34 GMT+01:00 Christian Convey :

> On Mon, Nov 28, 2016 at 9:28 PM, Pavel Stehule 
> wrote:
>
>> We now support XPath function - JSONPath is similar to XPath - it is
>> better for user, because have to learn only one language.
>>
>
> I'm not sure I understand.
>
> Are you suggesting that we use XPath, not JSONPath, as our language for
> json-path expressions?
>

surely not.

follow ANSI/SQL :)

Pavel


>
> ​- C
>

Re: [HACKERS] Tackling JsonPath support

[Christian Convey]

On Mon, Nov 28, 2016 at 9:28 PM, Pavel Stehule 
wrote:

> We now support XPath function - JSONPath is similar to XPath - it is
> better for user, because have to learn only one language.
>

I'm not sure I understand.

Are you suggesting that we use XPath, not JSONPath, as our language for
json-path expressions?

​- C

Re: [HACKERS] Tackling JsonPath support

[Pavel Stehule]

2016-11-29 4:00 GMT+01:00 David G. Johnston :

> On Mon, Nov 28, 2016 at 7:38 PM, Christian Convey <
> christian.con...@gmail.com> wrote:
>
>> On Mon, Nov 28, 2016 at 6:26 PM, Nico Williams 
>> wrote:
>>
>>> While XPath is expressive and compact, XSLT
>>> is rather verbose; jq is as expressive as XSLT, but with the compact
>>> verbosity of XPath.
>>>
>>
>> Instead, your point was that jq seems to have many advantages over
>> json-path in general, and therefore PG should offer jq instead or, or in
>> addition to, json-path.
>>
>>
> IMO jq is considerably closer to XSLT than XPath - which leads me to
> figure that since xml has both that JSON can benefit from jq and
> json-path.  I'm not inclined to dig too deep here but I'd rather take jq in
> the form of "pl/jq" and have json-path (abstractly) as something that you
> can use like "pg_catalog.get_value(json, json-path)"
>

I am not against to this idea. The jq and similar environments can have
sense in JSON NoSQL databases. Using it in relation database  in searching
functions is a overkill.

Regards

Pavel



>
> ​David J.
> ​
>
>

Re: [HACKERS] GiST support for UUIDs

[Chris Bandy]

On Mon, Nov 28, 2016 at 4:04 PM, Tom Lane  wrote:
>
> What I would suggest is that you forget the union hack and just use
> memcmp in all the comparison functions.  It's not likely to be worth
> the trouble to try to get those calls to be safely aligned.  The
> only place where you need go to any trouble is in uuid_parts_distance,
> which could be redesigned to memcpy a not-necessarily-aligned source
> into a local uint64[2] array and then byte-swap if needed.

Done. I only have one architecture to test on (Linux, Intel x86_64) so
I must defer to others in this regard.

> I don't entirely see the point of making pg_uuid_t an opaque struct when
> the only interesting thing about it, namely UUID_LEN, is exposed anyway.
> So my inclination would be to just do
>
> typedef struct pg_uuid_t
> {
> unsigned char data[UUID_LEN];
> } pg_uuid_t;
>
> in uuid.h and forget the idea of it being opaque.

Done.

> Also, the patch could be made a good deal smaller (and easier to review)
> in the wake of commit 40b449ae8: you no longer need to convert
> btree_gist--1.2.sql into btree_gist--1.3.sql, just leave it alone and add
> btree_gist--1.2--1.3.sql.  That way we don't have to worry about whether
> the upgrade script matches the change in the base script.

Done.


-- Chris


btree_gist_uuid_8.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] Tackling JsonPath support

[Pavel Stehule]

2016-11-29 2:50 GMT+01:00 Christian Convey :

> On Mon, Nov 28, 2016 at 11:23 AM, Nico Williams 
> wrote:
> ...
>
>> JSON Path is not expressive enough (last I looked) and can be mapped
>> onto jq if need be anyways.
>>
>
> ​Hi Nico,
>
> Could you please clarify what you mean by "not expressive enough"?
>
> I ask because I've been struggling to identify clear requirements for the
> json-path functionality I'm trying to provide.  It sounds like perhaps you
> have something concrete in mind.
>
> Since I myself have no need currently for this functionality, I'm left
> guessing about hypothetical users of it.​  My current mental model is:
>
> (a) Backend web developers.  AFAICT, their community has mostly settled on
> the syntax/semantics proposed by Stefan Groessner.  It would probably be
> unkind for PG's implementation to deviate from that without a good reason.
>
> (b) PG hackers who will eventually implement the ISO SQL standard
> operators.  In the standards-committee meeting notes I've seen, it seemed
> to me that they were planning to define some operators in terms of
> json-path expression.  So it would probably be good if whatever json-path
> function I implement turns out to comply with that standard, so that the
> PG-hackers can use it as a building block for their work.
>
> (c) Pavel.  (I'm still somewhat unclear on what has him interested in
> this, and what his specific constraints are.)
>

My target is simple - 1. to have good ANSI/SQL support, 2. to have good
JSON to relation mapping function - ANSI/SQL JSONTABLE does it.

We now support XPath function - JSONPath is similar to XPath - it is better
for user, because have to learn only one language.

Regards

Pavel

>
> - Christian
>
>

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

[Mithun Cy]

Sorry I took some time on this please find latest patch with addressed
review comments. Apart from fixes for comments I have introduced a new GUC
variable for the pg_autoprewarm "buff_dump_interval". So now we dump the
buffer pool metadata at every buff_dump_interval secs. Currently
buff_dump_interval can be set only at startup time. I did not choose to do
the dumping at checkpoint time, as it appeared these 2 things are not much
related and keeping it independent would be nice for usage. Also overhead
of any communication between them can be avoided.

On Fri, Oct 28, 2016 at 1:45 AM, Jim Nasby  wrote:
 > IMO it would be better to add this functionality to pg_prewarm instead
of creating another contrib module. That would reduce confusion and reduce
 > the amount of code necessary.

I have merged pg_autoprewarm module into pg_prewarm, This is just the
directory merge, Functionality merge is not possible pg_prewarm is just a
utility function with specific signature to load a specific relation at
runtime, where as pg_autoprewarm is a bgworker which dumps current buffer
pool and load it during startup time.

> Presumably the first 4 numbers will vary far less than blocknum, so it's
probably worth reversing the order here (or at least put blocknum first).

function sort_cmp_func is for qsort so orderly comparison is needed to say
which is bigger or smaller, If we put blocknum first, we cannot decide same.

> AFAICT this isn't necessary since palloc will error itself if it fails.

Fixed.

> Since there's no method to change DEFAULT_DUMP_FILENAME, I would call it
what it is: DUMP_FILENAME.

Fixed.

> Also, maybe worth an assert to make sure there was enough room for the
complete filename. That'd need to be a real check if this was configurable
> anyway.

I think if we make it configurable I think I can put that check.

 > + if (!avoid_dumping)
 > +   dump_now();
 > Perhaps that check should be moved into dump_now() itself...

 Fixed.

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


pg_autoprewarm_02.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] UNDO and in-place update

[Amit Kapila]

On Mon, Nov 28, 2016 at 11:01 PM, Robert Haas  wrote:
> On Sun, Nov 27, 2016 at 10:44 PM, Amit Kapila  wrote:
>> On Mon, Nov 28, 2016 at 4:50 AM, Robert Haas  wrote:
>>> Well, my original email did contain a discussion of the need for
>>> delete-marking.  I said that you couldn't do in-place updates when
>>> indexed columns were modified unless the index AMs had support for
>>> delete-marking, which is the same point you are making here.
>>
>> Sorry, I had not read that part earlier, but now that I read it, I
>> think there is a slight difference in what I am saying.   I thought
>> along with delete-marking, we might need transaction visibility
>> information in the index as well.
>
> I think we need to avoid putting the visibility information in the
> index because that will make the index much bigger.
>

I agree that there will be an increase in index size, but it shouldn't
be much if we have transaction information (and that too only active
transactions) at page level instead of at tuple level.  I think the
number of concurrent writes on the index will be lesser as compared to
the heap.  There are a couple of benefits of having visibility
information in the index.

a. Heap and index could be independently cleaned up and in most cases
by foreground transactions only.  The work for vacuum will be quite
less as compared to now.  I think this will help us in avoiding the
bloat to a great degree.

b. Improved index-only scans, because now we don't need visibility map
of the heap to check tuple visibility.

c. Some speed improvements for index scans can also be expected
because with this we don't need to perform heap fetch for invisible
index tuples.

>  It would be a
> shame to get the visibility index out of the heap (as this design
> does) only to be forced to add it to the index.  Note that,
> percentage-wise, it's quite a bit worse to have visibility information
> in the index than in the heap, because the index tuples are going to
> be much narrower than the heap tuples, often just one column.  (The
> cost of having this information in the heap can be amortized across
> the whole width of the table.)
>

I think with proposed design there is a good chance that for many of
the index scans, there is a need for the extra hop to decide
visibility of index tuple. I think as of today, during index scan if
the corresponding heap page is not-all-visible, then we check the
corresponding heap tuple to decide about the visibility of index
tuple, whereas with proposed design, I think it first needs to check
heap page and then TPD, if there is more than one transaction
operating on page.

> I don't have the whole design for the delete-marking stuff worked out
> yet.  I'm thinking we could have a visibility map where the bit for
> each page is 1 if the page certainly has no pending UNDO and 0 if it
> might have some.  In addition, if a tuple is deleted or the indexed
> column value is changed, we delete-mark the associated index entries.
> If we later discover that the page has no current UNDO (i.e. is
> all-visible) and the tuple at a given TID matches our index entry, we
> can clear the delete-mark for that index entry.  So, an index-only
> scan rechecks the heap if the tuples is delete-marked OR the
> visibility-map bit for the page is not set; if neither is the case, it
> can assume the heap tuple is visible.
>

Such a design will work, but I think this is more work to mark the
tuple as Dead as compared to current system.

> Another option would be to get rid of the visibility map and rely only
> on the delete-marks.  If we did that, then tuples would have to be
> delete-marked when first inserted since they wouldn't be all-visible
> until sometime after the commit of the inserting transaction.
>

The previous idea sounds better.

>
>>>  That's OK, but we're in real trouble if
>>> step-3 inserted an additional index tuple pointing to that chain
>>> rather than simply noting that one already exists.  If it adds an
>>> additional one, then we'll visit two index tuples for that TID.  Each
>>> time, the visibility information in UNDO will cause us to return the
>>> correct tuple, but we've erred by returning it twice (once per index
>>> entry) rather than just once.
>>
>> Why will scan traverse the UNDO chain if it starts after step-3 commit?
>
> Why wouldn't it?  I think that if an index scan hits a page with an
> UNDO chain, it always need to traverse the whole thing.
>

If you notice the scan has started after all the writes have
committed, so what is the guarantee that there will be a UNDO chain?
Yes, it could be there if there is some live transaction which started
before step, otherwise, I am not sure if we can guarantee that UNDO
chain will be present.

-- 
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:

[HACKERS] Thinko in set_rel_consider_parallel()

[Amit Langote]

The following looks like a thinko, which fixed in attached:

-Oid proparallel = func_parallel(...
+charproparallel = func_parallel(...

Thanks,
Amit
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index e42ef98..ec89b6a 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -537,7 +537,7 @@ set_rel_consider_parallel(PlannerInfo *root, RelOptInfo *rel,
 			 */
 			if (rte->tablesample != NULL)
 			{
-Oid			proparallel = func_parallel(rte->tablesample->tsmhandler);
+char	proparallel = func_parallel(rte->tablesample->tsmhandler);
 
 if (proparallel != PROPARALLEL_SAFE)
 	return;

-- 
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] Mention column name in error messages

[Michael Paquier]

On Mon, Nov 7, 2016 at 10:26 AM, Franck Verrot  wrote:
>
>
> On Sun, Nov 6, 2016 at 1:13 PM, Tom Lane  wrote:
>>
>> > The original intent of that patch tried to cover the case where we
>> > insert
>> > records
>> > made of dozens columns sharing the same type definition, and trying to
>> > understand
>> > what is going on, at a glance, when we debugged something like this:
>> > ...
>> > Relying on the cursor seems to be of little help I'm afraid.
>>
>> Well, it would be an improvement over what we've got now.  Also, a feature
>> similar to what I suggested would help in localizing many types of errors
>> that have nothing to do with coercion to a target column type.
>
>
> Yes, it's a neat improvement in any case.

I have marked this patch as "Returned with Feedback" as it got its
share of reviews.
-- 
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] Password identifiers, protocol aging and SCRAM protocol

[Michael Paquier]

On Fri, Nov 18, 2016 at 2:51 AM, Michael Paquier
 wrote:
> On Thu, Nov 17, 2016 at 8:12 AM, Robert Haas  wrote:
>> So, the problem isn't Darwin-specific.  I experimented with this on
>> Linux and found Linux does the same thing with libpgcommon_srv.a that
>> macOS does: a file in the archive that is totally unused is omitted
>> from the postgres binary.  In Linux, however, that doesn't prevent
>> pgcrypto from compiling anyway.  It does, however, prevent it from
>> working.  Instead of failing at compile time with a complaint about
>> missing symbols, it fails at load time.  I think that's because macOS
>> has -bundle-loader and we use it; without that, I think we'd get the
>> same behavior on macOS that we get on Windows.
>
> Yes, right. I recall seeing the regression tests failing with pgcrypto
> when doing that. Though I did not recall if this was specific to macos
> or Linux when I looked again at this patch yesterday. When testing
> again yesterday I was able to make the tests of pgcrypto to pass, but
> perhaps my build was not in a clean state...
>
>> 1. Rejigger things so that we don't build libpgcommon_srv.a in the
>> first place, and instead add $(top_builddir)/src/common to
>> src/backend/Makefile's value of SUBDIRS.  With appropriate adjustments
>> to src/common/Makefile, this should allow us to include all of the
>> object files on the linker command line individually instead of
>> building an archive library that is then used only for the postgres
>> binary itself anyway.  Then, things wouldn't get dropped.
>>
>> 2. Just postpone committing this patch until we're ready to use the
>> new code in the backend someplace (or add a dummy reference to it
>> someplace).
>
> At the end this refactoring makes sense because it will be used in the
> backend with the SCRAM engine, so we could just wait for 2 instead of
> having some workarounds. This is dropping the ball for later and there
> will be already a lot of work for the SCRAM core part, though I don't
> think that the SHA2 refactoring will change much going forward.
>
> Option 3 would be to do things the patch does it, aka just compiling
> pgcrypto using the source files directly and put a comment to revert
> that once the APIs are used in the backend. I can guess that you don't
> like that.

Nothing more will likely happen in this CF, so I have moved it to
2017-01 with the same status of "Needs Review".
-- 
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] Renaming of pg_xlog and pg_clog

[Michael Paquier]

On Tue, Nov 22, 2016 at 8:35 PM, Haribabu Kommi
 wrote:
> Hi Craig,
>
> This is a gentle reminder.
>
> you assigned as reviewer to the current patch in the 11-2016 commitfest.
> But you haven't shared your review yet. Please share your review about
> the patch. This will help us in smoother operation of commitfest.
>
> Please Ignore if you already shared your review.

I have moved this CF entry to 2017-01, the remaining, still unreviewed
patch are for renaming pg_subxact and pg_clog.
-- 
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] Forbid use of LF and CR characters in database and role names

[Michael Paquier]

On Fri, Nov 25, 2016 at 4:41 PM, Michael Paquier
 wrote:
> On Fri, Nov 25, 2016 at 4:02 PM, Ideriha, Takeshi
>  wrote:
>> I applied your fixed patch and new one, and confirmed the applied source 
>> passed the tests successfully. And I also checked manually the error 
>> messages were emitted successfully when cr/lf are included in dbname or 
>> rolename or data_directory.
>>
>> AFAICT, this patch satisfies the concept discussed before. So I’ve switched 
>> this patch “Ready for Committer”.
>
> Thanks for the review, Ideriha-san. (See you next week perhaps?)

Patch moved to CF 2017-01.
-- 
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] Quorum commit for multiple synchronous replication.

[Michael Paquier]

On Mon, Nov 28, 2016 at 8:03 PM, Masahiko Sawada  wrote:
> On Sat, Nov 26, 2016 at 10:27 PM, Michael Paquier
>  wrote:
>> On Tue, Nov 15, 2016 at 7:08 PM, Masahiko Sawada  
>> wrote:
>>> Attached latest version patch incorporated review comments. After more
>>> thought, I agree and changed the value of standby priority in quorum
>>> method so that it's not set 1 forcibly. The all standby priorities are
>>> 1 If s_s_names = 'ANY(*)'.
>>> Please review this patch.
>>
>> Sorry for my late reply. Here is my final lookup.
>
> Thank you for reviewing!
>
>>  
>> -num_sync ( > class="parameter">standby_name [, ...] )
>> +[ANY] num_sync (
>> standby_name [, ...] )
>> +FIRST num_sync (
>> standby_name [, ...] )
>>  standby_name [, ...
>> This can just be replaced with [ ANY | FIRST ]. There is no need for
>> braces as the keyword is not mandatory.
>>
>> +is the name of a standby server.
>> +FIRST and ANY specify the method used by
>> +the master to control the standby servres.
>>  
>> s/servres/servers/.
>>
>> if (priority == 0)
>> values[7] = CStringGetTextDatum("async");
>> else if (list_member_int(sync_standbys, i))
>> -   values[7] = CStringGetTextDatum("sync");
>> +   values[7] = SyncRepConfig->sync_method == SYNC_REP_PRIORITY ?
>> +   CStringGetTextDatum("sync") : 
>> CStringGetTextDatum("quorum");
>> else
>> values[7] = CStringGetTextDatum("potential");
>> This can be simplified a bit as "quorum" is the state value for all
>> standbys with a non-zero priority when the method is set to
>> SYNC_REP_QUORUM:
>> if (priority == 0)
>> values[7] = CStringGetTextDatum("async");
>> +   else if (SyncRepConfig->sync_method == SYNC_REP_QUORUM)
>> +   values[7] = CStringGetTextDatum("quorum");
>> else if (list_member_int(sync_standbys, i))
>> values[7] = CStringGetTextDatum("sync");
>> else
>>
>> SyncRepConfig data is made external to syncrep.c with this patch as
>> walsender.c needs to look at the sync method in place, no complain
>> about that after considering if there could be a more elegant way to
>> do things without this change.
>
> Agreed.
>
>> While reviewing the patch, I have found a couple of incorrectly shaped
>> sentences, both in the docs and some comments. Attached is a new
>> version with this word-smithing. The patch is now switched as ready
>> for committer.
>
> Thanks. I found a typo in v7 patch, so attached latest v8 patch.

Moved patch to CF 2017-01, with same status "Ready for committer".
-- 
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] WAL consistency check facility

[Michael Paquier]

On Wed, Nov 16, 2016 at 2:15 AM, Michael Paquier
 wrote:
> On Tue, Nov 15, 2016 at 7:50 AM, Kuntal Ghosh
>  wrote:
>> I've modified the guc parameter name as wal_consistency_check (little
>> hesitant for a participle in suffix :) ). Also, updated the sgml and
>> variable name accordingly.
>
> The changes look good to me.

Moved to CF 2017-01, as no committers have showed up yet :(
-- 
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] pg_dump, pg_dumpall and data durability

[Michael Paquier]

On Mon, Nov 14, 2016 at 6:07 PM, Michael Paquier
 wrote:
> On Mon, Nov 14, 2016 at 6:04 PM, Albe Laurenz  wrote:
>> Michael Paquier wrote:
>>> Meh. I forgot docs and --help output updates.
>>
>> Looks good, except that you left the "N" option in the getopt_long
>> call for pg_dumpall.  I fixed that in the attached patch.
>
> No, v5 has removed it, but it does not matter much now...
>
>> I'll mark the patch "ready for committer".
>
> Thanks!

Moved to CF 2017-01.
-- 
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] Re: [sqlsmith] FailedAssertion("!(XLogCtl->Insert.exclusiveBackup)", File: "xlog.c", Line: 10200)

[Michael Paquier]

On Thu, Nov 17, 2016 at 1:18 PM, Michael Paquier
 wrote:
> On Mon, Nov 7, 2016 at 6:18 PM, Kyotaro HORIGUCHI
>  wrote:
>> I will mark this as "Ready for Committer".
>
> I have just noticed that Robert has switched this patch to "needs
> review" by mistake (I think that there was a mistake with another
> patch), so I have switched it back to "Ready for committer". I agree
> with Horiguchi-san that this seems adapted, as it got some review and
> some love.

Moved to next CF.
-- 
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] Fix checkpoint skip logic on idle systems by tracking LSN progress

[Michael Paquier]

On Tue, Nov 22, 2016 at 6:27 PM, Kyotaro HORIGUCHI
 wrote:
> I have marked this as ready for committer again.

And moved to next CF for now.
-- 
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] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Michael Paquier]

On Wed, Nov 16, 2016 at 12:45 PM, Christian Ullrich
 wrote:
> I also did a debug build with 1+2+4 that came in at 84 μs/iteration.

Moved to next CF. Christian, perhaps this patch should have an extra
round of reviews?
-- 
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] Proposal: scan key push down to heap [WIP]

[Andres Freund]

On 2016-11-28 09:55:00 -0500, Robert Haas wrote:
> I think we should go with this approach.  I don't think it's a good
> idea to have the heapam layer know about executor slots.

I agree that that's not pretty.

> Even though
> it's a little sad to pass up an opportunity for a larger performance
> improvement, this improvement is still quite good.

It's imo not primarily about a larger performance improvement, but about
avoid possible regressions. Doubling deforming of wide tuples will have
noticeable impact on some workloads. I don't think we can disregard
that.

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] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled

[Michael Paquier]

On Tue, Nov 22, 2016 at 1:58 PM, Tsunakawa, Takayuki
 wrote:
> From: Craig Ringer [mailto:cr...@2ndquadrant.com]
>> You meant CheckTokenMembership().
>
> Yes, my typo in the mail.
>
>> The proposed patch does need to be checked with:
>
> I understood you meant by "refuse to run" that postgres.exe fails to start 
> below.  Yes, I checked it on Win10.  I don't have access to WinXP/2003 - 
> Microsoft ended their support.
>
> if (pgwin32_is_admin())
> {
> write_stderr("Execution of PostgreSQL by a user with 
> administrative permissions is not\n"
>  "permitted.\n"
>  "The server must be started under an 
> unprivileged user ID to prevent\n"
>  "possible system security compromises.  See the 
> documentation for\n"
>   "more information on how to properly start 
> the server.\n");
> exit(1);
> }

I have moved that to next CF. The refactoring patch needs more testing
but the basic fix patch could be applied.
-- 
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] Time to up bgwriter_lru_maxpages?

[Michael Paquier]

On Tue, Nov 29, 2016 at 6:20 AM, Jim Nasby  wrote:
> On 11/28/16 11:53 AM, Joshua D. Drake wrote:
>>
>> On 11/28/2016 11:40 AM, Jim Nasby wrote:
>>>
>>> With current limits, the most bgwriter can do (with 8k pages) is 1000
>>> pages * 100 times/sec = 780MB/s. It's not hard to exceed that with
>>> modern hardware. Should we increase the limit on bgwriter_lru_maxpages?
>>
>>
>> Considering a single SSD can do 70% of that limit, I would say yes.
>
>
> Next question becomes... should there even be an upper limit?

Looking at the log history the current default dates of cfeca621, it
would be time to raise the bar a little bit more. Even an utterly high
value could make sense for testing.
-- 
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] Proposal: scan key push down to heap [WIP]

[Dilip Kumar]

On Mon, Nov 28, 2016 at 8:25 PM, Robert Haas  wrote:
> I think we should go with this approach.  I don't think it's a good
> idea to have the heapam layer know about executor slots.  Even though
> it's a little sad to pass up an opportunity for a larger performance
> improvement, this improvement is still quite good.

I agree.

However, there's a
> fair amount of this patch that doesn't look right:
>
> - The changes to heapam.c shouldn't be needed any more.  Ditto
> valid.h, relscan.h, catcache.c and maybe some other stuff.

Actually we want to call slot_getattr instead heap_getattr, because of
problem mentioned by Andres upthread and we also saw in test results.

Should we make a copy of HeapKeyTest lets say ExecKeyTest and keep it
under executor ?

ExecKeyTest will be same as HeapKeyTest but it will call slot_getattr
instead of heap_getattr.

>
> - get_scankey_from_qual() should be done at plan time, not execution
> time.  Just as index scans already divide up quals between "Index
> Cond" and "Filter" (see ExplainNode), I think Seq Scans are now going
> to need to something similar.  Obviously, "Index Cond" isn't an
> appropriate name for things that we test via HeapKeyTest, but maybe
> "Heap Cond" would be suitable. That's going to be a fair amount of
> refactoring, since the "typedef Scan SeqScan" in plannodes.h is going
> to need to be replaced by an actual new structure definition.
>
Okay.
> - get_scankey_from_qual()'s prohibition on variable-width columns is
> presumably no longer necessary with this approach, right?

Correct.
>
> - Anything tested in SeqNext() will also need to be retested in
> SeqRecheck(); otherwise, the test will be erroneously skipped during
> EPQ rechecks.
Okay..


-- 
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] Tackling JsonPath support

[Nico Williams]

On Mon, Nov 28, 2016 at 08:00:46PM -0700, David G. Johnston wrote:
> IMO jq is considerably closer to XSLT than XPath - which leads me to figure
> that since xml has both that JSON can benefit from jq and json-path.  I'm
> not inclined to dig too deep here but I'd rather take jq in the form of
> "pl/jq" and have json-path (abstractly) as something that you can use like
> "pg_catalog.get_value(json, json-path)"

JSONPath looks a lot like a small subset of jq.  Here are some examples:

JSONPath|   jq
---

$.store.book[0].title   | .store.book[0].title
$['store']['book'][0]['title']  | .["store"]["book"][0]["title"]
$..author   | ..|.author
$.store.*   | .store[]
$.store..price  | .store|..|.price?
$..book[2]  | [..|.book?][2]
$..book[?(@.isbn)]  | ..|.book?|select(.isbn)
$..book[?(@.price<10)]  | ..|.book?|select(.price<10)
$..*| ..?

Of course, jq can do much more than this.  E.g.,

# Output [, ] of all books with an ISBN:
..|.book?|select(.isbn)|[.title,.price]

# Output the average price of books with ISBNs appearing anywhere in
# the input document:
reduce
  (..|.book?|select(.isbn)|.price) as $price
  (
   # Initial reduction state:
   {price:0,num:0};
   # State update
   .price = (.price * .num + $price) / (.num + 1) | .num += 1) |
# Extract average price
.price

Of course one could just wrap that with a function:

def avg(pathexp; cond; v):
  reduce (pathexp | select(cond) | v) as $v
({v: 0, c: 0};
 .v = (.v * .c + $v) / (.c + 1) | .c += 1) | v;

# Average price of books with ISBNs:
avg(..|.book?; .isbn; .price)

# Average price of all books:
avg(..|.book?; true; .price)

There's much, much more.

Note that jq comes with a C implementation.  It should be easy to make
bindings to it from other programming language run-times.

Nico
-- 


-- 
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] Tackling JsonPath support

[David G. Johnston]

On Mon, Nov 28, 2016 at 7:38 PM, Christian Convey <
christian.con...@gmail.com> wrote:

> On Mon, Nov 28, 2016 at 6:26 PM, Nico Williams 
> wrote:
>
>> While XPath is expressive and compact, XSLT
>> is rather verbose; jq is as expressive as XSLT, but with the compact
>> verbosity of XPath.
>>
>
> Instead, your point was that jq seems to have many advantages over
> json-path in general, and therefore PG should offer jq instead or, or in
> addition to, json-path.
>
>
IMO jq is considerably closer to XSLT than XPath - which leads me to figure
that since xml has both that JSON can benefit from jq and json-path.  I'm
not inclined to dig too deep here but I'd rather take jq in the form of
"pl/jq" and have json-path (abstractly) as something that you can use like
"pg_catalog.get_value(json, json-path)"

​David J.
​

Re: [HACKERS] Tackling JsonPath support

[Nico Williams]

On Mon, Nov 28, 2016 at 06:38:55PM -0800, Christian Convey wrote:
> On Mon, Nov 28, 2016 at 6:26 PM, Nico Williams 
> wrote:
> >
> 
> Thanks for the explanation.  It sounds like your original point was NOT
> that json-path isn't sufficient for "${specific use X}".

The only uses of SQL w/ JSON I've seen so far in live action are to
implement EAV schemas on PostgreSQL.  Since PostgreSQL lacks an ANY
type... using the hstore or jsonb to store data that would otherwise
require an ANY type is the obvious thing to do.  Naturally this use
doesn't need deeply nested JSON data structures, so even JSONPath is
overkill for it!

However, there are use cases I can imagine:

 - generating complex JSON from complex (e.g., recursive) SQL data where
   the desired JSON "schema" is not close to the SQL schema

   I've used jq a *lot* to convert schemas.  I've also use XSLT for the
   same purpose.  I've also used SQL RDBMSes and jq together a fair bit,
   either having jq consume JSON documents to output INSERT and other
   statements, or having a SQL application output JSON that I then
   convert to an appropriate schema using jq.

   Naturally I can keep using these two tools separately.  There's not
   much to gain from integrating them for this particular sort of
   use-case.

 - handling JSON documents with very loose schemata, perhaps arbitrary
   JSON documents, embedded in a SQL DB

   I've not needed to do this much, so I have no specific examples.
   But, of course, one reason I've not needed to do this is that today
   it kinda can't be done with enough expressivity.

There are many use-cases for general-purpose programming languages, and
even for very widely-applicable domain-specific programming language.

It's especially difficult to name a specific use-case for a language
that doesn't exist -- in this case that would be SQL + (jq and/or
JSONPath).

> Instead, your point was that jq seems to have many advantages over
> json-path in general, and therefore PG should offer jq instead or, or in
> addition to, json-path.
> 
> Is that what you're saying?

Roughly, yes.  The distinct advantage is that jq is much more general
and expressive, not unlike SQL itself.

> > Hmm?
> 
> Context: The reason I'm trying to work on a json-path implementation is
> that Pavel Stehule suggested it as a good first PG-hacking project for me.
> At the time, it sounded like he had a use for the feature.

I see.  I understand that.  If you've already made a significant
investment, then I don't blame you for not wanting to risk it.  On the
other hand, if melding jsonb and jq happens to be easy, then you'll get
much more bang from it for your investment.  Naturally, you do what you
prefer, and if the reality on the ground is JSONPath, then so be it.  If
I had time and felt sufficiently strongly, I'd contribute jq
integration; as it is I don't, and beggars can't be choosers.

Nico
-- 


-- 
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] Tackling JsonPath support

[Christian Convey]

On Mon, Nov 28, 2016 at 6:26 PM, Nico Williams 
wrote:

> On Mon, Nov 28, 2016 at 05:50:40PM -0800, Christian Convey wrote:
> > On Mon, Nov 28, 2016 at 11:23 AM, Nico Williams 
> > wrote:
> > ...
> > > JSON Path is not expressive enough (last I looked) and can be mapped
> > > onto jq if need be anyways.
> >
> > Hi Nico,
> >
> > Could you please clarify what you mean by "not expressive enough"?
>
> jq is a functional language that has these and other features:
>
>  - recursion
>  - generators
>  - lazy evaluation (of sorts)
>  - path expressions
>  - math functionality (libm, basically)
>  - reduction
>  - functions
>  - and other things
>
> (jq does not have higher-order functions in that functions cannot return
> functions and functions are not values, though it does have closures.)
>
> jq is and feels a lot like a SQL, but for JSON.
>
> > I ask because I've been struggling to identify clear requirements for the
> > json-path functionality I'm trying to provide.  It sounds like perhaps
> you
> > have something concrete in mind.
>
> SQL imposes structure on data.  Recursion makes SQL structure looser in
> the sense that it may not be easy or possible to express certain
> desirable schema constraints in SQL terms without resorting to triggers,
> say.  Storing documents in XML, JSON, or other such recursion-friendly
> formats (perhaps in semantically equivalent but query-optimized forms)
> is also a way to avoid strict structure (thus one needs schema
> validators for XML, for example).
>
> Less rigid schema constraints do not and should not preclude powerful
> query languages.
>
> One could convert such documents to a SQL EAV schema, if one has an
> RDBMS with an ANY type (e.g., something like SQLite3's duck typing), and
> then use SQL to query them.  But that may be more difficult to use than
> a SQL with support for XML/JSON/... and query sub-languages for those.
>
> SQL is very powerful.  One might like to have similarly powerful,
> format-specific query languages for documents stored in XML, JSON,
> etcetera, in a SQL RDBMS.  jq is such a language, for JSON documents.
> Ditto XPath/XSLT, for XML.  While XPath is expressive and compact, XSLT
> is rather verbose; jq is as expressive as XSLT, but with the compact
> verbosity of XPath.
>
> > Since I myself have no need currently for this functionality, I'm left
> > guessing about hypothetical users of it.  My current mental model is:
>
> That's a bit like asking what is the use for SQL :^)  The point is that
> SQL is a powerful query language, and so is jq.  Each is appropriate to
> its own domain; both could be used together.
>

Thanks for the explanation.  It sounds like your original point was NOT
that json-path isn't sufficient for "${specific use X}".

Instead, your point was that jq seems to have many advantages over
json-path in general, and therefore PG should offer jq instead or, or in
addition to, json-path.

Is that what you're saying?

​...


> > (c) Pavel.  (I'm still somewhat unclear on what has him interested in
> this,
> > and what his specific constraints are.)
>
> Hmm?
>

​Context: The reason I'm trying to work on a json-path implementation is
that Pavel Stehule suggested it as a good first PG-hacking project for me.
At the time, it sounded like he had a use for the feature.

- C

Re: [HACKERS] Tackling JsonPath support

[Nico Williams]

On Mon, Nov 28, 2016 at 05:50:40PM -0800, Christian Convey wrote:
> On Mon, Nov 28, 2016 at 11:23 AM, Nico Williams 
> wrote:
> ...
> > JSON Path is not expressive enough (last I looked) and can be mapped
> > onto jq if need be anyways.
> 
> Hi Nico,
> 
> Could you please clarify what you mean by "not expressive enough"?

jq is a functional language that has these and other features:

 - recursion
 - generators
 - lazy evaluation (of sorts)
 - path expressions
 - math functionality (libm, basically)
 - reduction
 - functions
 - and other things

(jq does not have higher-order functions in that functions cannot return
functions and functions are not values, though it does have closures.)

jq is and feels a lot like a SQL, but for JSON.

> I ask because I've been struggling to identify clear requirements for the
> json-path functionality I'm trying to provide.  It sounds like perhaps you
> have something concrete in mind.

SQL imposes structure on data.  Recursion makes SQL structure looser in
the sense that it may not be easy or possible to express certain
desirable schema constraints in SQL terms without resorting to triggers,
say.  Storing documents in XML, JSON, or other such recursion-friendly
formats (perhaps in semantically equivalent but query-optimized forms)
is also a way to avoid strict structure (thus one needs schema
validators for XML, for example).

Less rigid schema constraints do not and should not preclude powerful
query languages.

One could convert such documents to a SQL EAV schema, if one has an
RDBMS with an ANY type (e.g., something like SQLite3's duck typing), and
then use SQL to query them.  But that may be more difficult to use than
a SQL with support for XML/JSON/... and query sub-languages for those.

SQL is very powerful.  One might like to have similarly powerful,
format-specific query languages for documents stored in XML, JSON,
etcetera, in a SQL RDBMS.  jq is such a language, for JSON documents.
Ditto XPath/XSLT, for XML.  While XPath is expressive and compact, XSLT
is rather verbose; jq is as expressive as XSLT, but with the compact
verbosity of XPath.

> Since I myself have no need currently for this functionality, I'm left
> guessing about hypothetical users of it.  My current mental model is:

That's a bit like asking what is the use for SQL :^)  The point is that
SQL is a powerful query language, and so is jq.  Each is appropriate to
its own domain; both could be used together.

> (a) Backend web developers.  AFAICT, their community has mostly settled on
> the syntax/semantics proposed by Stefan Groessner.  It would probably be
> unkind for PG's implementation to deviate from that without a good reason.

I can't speak for the community.  I wouldn't take it personally that jq
be not chosen, nor any other proposal of mine.  If it's politically
easier, then do that.

> (b) PG hackers who will eventually implement the ISO SQL standard
> operators.  In the standards-committee meeting notes I've seen, it seemed
> to me that they were planning to define some operators in terms of
> json-path expression.  So it would probably be good if whatever json-path
> function I implement turns out to comply with that standard, so that the
> PG-hackers can use it as a building block for their work.

These could still be implemented (e.g., using jq itself).

> (c) Pavel.  (I'm still somewhat unclear on what has him interested in this,
> and what his specific constraints are.)

Hmm?

Nico
-- 


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

[HACKERS] Corner-case improvement to eqjoinsel_semi

[Tom Lane]

There's a complaint in bug #14438 about poor estimation of join size
for a semijoin whose inner side is empty.  I think the root of it is that,
having no statistics for the empty table, eqjoinsel_semi distrusts its
estimate of the number of distinct values on the inner side, and falls
back to a conservative calculation that has little to do with reality.
However, it's not really true that the nd2 estimate is based on nothing at
all, because we clamped it to be no more than the estimated size of the
inner relation (which we knew to be small).  If we go ahead and use that
number as a valid estimate, we get a far better selectivity estimate ---
in the bug's example, the join size estimate goes from 16000-some to 2,
which is a tad closer to the correct value of 0.

Hence, I propose the attached patch.  This would kick in whenever the
inner side of a semi/antijoin has no statistics and an estimated size
of less than 200 rows.

Thoughts?

regards, tom lane

diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index 56943f2..d331adf 100644
*** a/src/backend/utils/adt/selfuncs.c
--- b/src/backend/utils/adt/selfuncs.c
*** eqjoinsel_semi(Oid operator,
*** 2511,2520 
  	 * We can apply this clamping both with respect to the base relation from
  	 * which the join variable comes (if there is just one), and to the
  	 * immediate inner input relation of the current join.
  	 */
  	if (vardata2->rel)
! 		nd2 = Min(nd2, vardata2->rel->rows);
! 	nd2 = Min(nd2, inner_rel->rows);
  
  	if (HeapTupleIsValid(vardata1->statsTuple))
  	{
--- 2511,2532 
  	 * We can apply this clamping both with respect to the base relation from
  	 * which the join variable comes (if there is just one), and to the
  	 * immediate inner input relation of the current join.
+ 	 *
+ 	 * If we clamp, we can consider that nd2 is not a bogus default estimate.
  	 */
  	if (vardata2->rel)
! 	{
! 		if (nd2 >= vardata2->rel->rows)
! 		{
! 			nd2 = vardata2->rel->rows;
! 			isdefault2 = false;
! 		}
! 	}
! 	if (nd2 >= inner_rel->rows)
! 	{
! 		nd2 = inner_rel->rows;
! 		isdefault2 = false;
! 	}
  
  	if (HeapTupleIsValid(vardata1->statsTuple))
  	{

-- 
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] Tackling JsonPath support

[Christian Convey]

On Mon, Nov 28, 2016 at 11:23 AM, Nico Williams 
wrote:
...

> JSON Path is not expressive enough (last I looked) and can be mapped
> onto jq if need be anyways.
>

​Hi Nico,

Could you please clarify what you mean by "not expressive enough"?

I ask because I've been struggling to identify clear requirements for the
json-path functionality I'm trying to provide.  It sounds like perhaps you
have something concrete in mind.

Since I myself have no need currently for this functionality, I'm left
guessing about hypothetical users of it.​  My current mental model is:

(a) Backend web developers.  AFAICT, their community has mostly settled on
the syntax/semantics proposed by Stefan Groessner.  It would probably be
unkind for PG's implementation to deviate from that without a good reason.

(b) PG hackers who will eventually implement the ISO SQL standard
operators.  In the standards-committee meeting notes I've seen, it seemed
to me that they were planning to define some operators in terms of
json-path expression.  So it would probably be good if whatever json-path
function I implement turns out to comply with that standard, so that the
PG-hackers can use it as a building block for their work.

(c) Pavel.  (I'm still somewhat unclear on what has him interested in this,
and what his specific constraints are.)

- Christian

Re: [HACKERS] Fix comment in build_simple_rel

[Amit Langote]

On 2016/11/29 3:57, Alvaro Herrera wrote:
> Amit Langote wrote:
>> Attached fixes reference in a comment to a non-existent function:
>>
>> s/GetRelationInfo/get_relation_info/g
> 
> Thanks, pushed.  get_relation_info() itself had been neglected when this
> responsibility was added onto it; I added an entry there too.

Thanks!

Regards,
Amit




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

Re: [HACKERS] Improving RLS planning

[Stephen Frost]

Tom,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > * Dean Rasheed (dean.a.rash...@gmail.com) wrote:
> >> +1 for that terminology and no renaming of fields.
> 
> > Agreed.
> 
> Here's an updated version of the RLS planning patch that gets rid of
> the incorrect interaction with Query.hasRowSecurity and adjusts
> terminology as agreed.

I've spent a fair bit of time going over this change to understand it,
how it works, and how it changes the way RLS and securiy barrier views
work.

Overall, I'm happy with how it works and don't see any serious issues
with the qual ordering or the general concept.  I did have a few
comments from my review:

> diff --git a/src/backend/optimizer/README b/src/backend/optimizer/README
> [...]
> + Additional rules will be needed to support safe handling of join quals
> + when there is a mix of security levels among join quals; for example, it
> + will be necessary to prevent leaky higher-security-level quals from being
> + evaluated at a lower join level than other quals of lower security level.
> + Currently there is no need to consider that since security-prioritized
> + quals can only be single-table restriction quals coming from RLS policies
> + or security-barrier views, and thus enforcement only needs to happen at
> + the table scan level.  With such extra rules, it should be possible to let
> + security-barrier views be flattened into the parent query, allowing more
> + flexibility of planning while still preserving required ordering of qual
> + evaluation.  But that will come later.

Are you thinking that we will be able to remove the cut-out in
is_simple_subquery() which currently punts whenever an RTE is marked as
security_barrier?  That would certainly be excellent as, currently, even
if everything involved is leakproof, we may end up with a poor choice of
plan because the join in the security barrier view must be performed
first.  Consider a case where we have two relativly large tables being
joined together in a security barrier view, but a join from outside
which is against a small table.  A better plan would generally be to
join with the smaller table first and then join the resulting small set
against the remaining large table.

Speaking of which, it seems like we should probably update the README to
include some mention, at least, of what we're doing today when it comes
to joins which involve security barrier entanglements.
  
> diff --git a/src/backend/optimizer/path/allpaths.c 
> b/src/backend/optimizer/path/allpaths.c
[...]
> ! /*
> !  * In addition to the quals inherited from the parent, we might 
> have
> !  * securityQuals associated with this particular child node.  
> (This
> !  * won't happen in inheritance cases, only with appendrels 
> originating
> !  * from UNION ALL.)  Pull them up into the baserestrictinfo for 
> the
> !  * child.  This is similar to process_security_barrier_quals() 
> for the
> !  * parent rel, except that we can't make any general deductions 
> from
> !  * such quals, since they don't hold for the whole appendrel.
> !  */

Right, this won't happen in inheritance cases because we explicitly
don't consider the quals of the children when querying through the
parent, similar to how we don't consider the GRANT-based permissions on
the child tables.  This is mentioned elsewhere but might make sense to
also mention it here, or at least say 'see expand_inherited_rtentry()'.

> *** subquery_push_qual(Query *subquery, Rang
> *** 2708,2714 
>   make_and_qual(subquery->jointree->quals, qual);
>   
>   /*
> !  * We need not change the subquery's hasAggs or hasSublinks 
> flags,
>* since we can't be pushing down any aggregates that weren't 
> there
>* before, and we don't push down subselects at all.
>*/
> --- 2748,2754 
>   make_and_qual(subquery->jointree->quals, qual);
>   
>   /*
> !  * We need not change the subquery's hasAggs or hasSubLinks 
> flags,
>* since we can't be pushing down any aggregates that weren't 
> there
>* before, and we don't push down subselects at all.
>*/

Seems like this change is unrelated to what this patch is about.  Not a
big deal, but did take me a second to realize that you were just
changing the case of the 'L' in hasSubLinks.

> +  * We also reject proposed equivalence clauses if they contain leaky 
> functions
> +  * and have security_level above zero.  The EC evaluation rules require us 
> to
> +  * apply certain tests at certain joining levels, and we can't tolerate
> +  * delaying any test on security_level grounds.  By rejecting candidate 
> clauses
> +  * that might require security delays, we ensure it's safe to apply an EC
> +  * 

Re: [HACKERS] matview incremental maintenance

[Nico Williams]

On Mon, Jun 17, 2013 at 07:41:15AM -0700, Kevin Grittner wrote:
> Since there seems to be interest in discussing incremental
> maintenance of materialized views *now*, I'm starting this thread
> to try to avoid polluting unrelated threads with the discussion.  I
> don't intend to spend a lot of time on it until the CF in progress
> completes, but at that point the work will start in earnest.  So
> I'll say where I'm at, and welcome anyone who has time to spare
> outside of the CF to comment or contribute ideas.

I have an implementation that supports updates, but it doesn't implement
automatic updates as described in the paper you cited.

> The paper at the core of the discussion can be found by searching
> for "maintaining views incrementally gupta mumick subrahmanian" --
> it's on both the ACM and CiteSeerX websites.  Of course, one
> doesn't need to understand that paper to discuss techniques for
> capturing the base deltas, but I'm hoping that's not what takes up
> most of the discussion.  I expect the most important discussions to
> be around how best to handle the "count(t)" (or count_t) column,
> what form should be use for intermediate results, how to modify or
> add execution nodes which know how to deal with the count, how to
> generate set operations to use those nodes, and how to modify the
> planner to choose the best plan for these operations.  Whether to
> pull the deltas off the WAL stream or stuff them into a tuplestore
> as they are written seems to me to be a relatively minor point.  If
> properly abstracted, the performance and complexity of alternatives
> can be compared.

Sure.  Automatically converting INSERTs/UPDATEs/DELETEs of MATERIALIZED
VIEW table sources is not trivial, though I've had some luck with a
particular multi-join query by just manually adding constraints from the
OLD.* and NEW.* rows, though unfortunately the PG query planner was
unable to find the obvious fast query plan (more on that in another
thread) and I ended up having to manually optimize the query using
WITH...

So at least for some queries it's easy enough to automatically propagate
constraints into the VIEW's query.  For some recursive queries it may
also be easy to propagate OLD.*/NEW.* into a seed.  Anyways, this is a
bit far afield though, as what I have managed so far is very useful even
without automatically producing updates based only on the VIEW's query.

Nor am I certain that automatically updating a materialized view is
always the right thing to do.

A case in point for me is an authorization system (which generally means
there's something of a transitive closure involved, which means
recursive queries).  In this system adding grants is cheap enough, but
massive revocation (e.g., deleting all of a user's entitlements, perhaps
by deleting the user and cascading the deletion) is not: it can be
faster to just refresh the view old-style than to update it dynamically!
(You noted this problem.)

The queries I use for dynamically updating the materialized views are
hand-optimized as mentioned above.  They are not too unlike what an
automatic system would have generated.

Granted, the problem partly is that an ORM is involved, which adds
obnoxious overhead: one statement per grant deletion versus one
statement for deleting all of them.  But it's still possible that at
some point it's best to refresh, even if an ORM were not involved.

> At the developer meeting last month, we talked about the special
> new count column for a bit, and everyone seemed to agree that
> adding such an animal, ...

What's that?

> Long term, timings for incremental maintenance that people would
> like to see (from most eager to least eager) are:
> 
> - as part of completing each statement, so that the affect on the
> matview is immediately visible to the transaction which modifies a
> supporting table, and becomes visible at commit to other
> transactions

That can be done now with hand-coded triggers, as I do in my use case.
Though I do leave some cases to a deferred refresh as mentioned above.

> - at transaction commit time, so that other transactions see the
> changes to the base tables and the referencing matviews at the same
> point in time

See above.

> - from a FIFO queue which is processed by a background process
> whenever data is present (possibly with pacing)

I do this.  I use NOTIFY/LISTEN and timeouts to drive refreshes as
needed.

> - from a FIFO queue based on a schedule, so that matviews are
> stable between applications and/or to avoid burdening the machine
> during peak periods

Ditto.

> - incremental update, or even full refresh, on an attempt to query
> a "stale" matview

I record and store (in a per-view history table) differences computed
during a refresh.

> - explicit request to apply incremental updates or refresh

I make this decision by using one trigger function or another.  One set
of trigger functions generates updates properly (by updating the
materialization table), and another merely marks 

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

[Tom Lane]

Andrew Dunstan  writes:
> On 11/28/2016 04:07 PM, Tom Lane wrote:
>> ... We've generally refrained from inventing any control flow
>> metacommands for psql

> I think it's really time we seriously considered adding some flow 
> control logic, though.

Yeah, maybe.  I'd be interested to see a fully worked out proposal
for that.

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] Types gbtreekey32 and gbtreekey16 aren't adequately aligned

[Tom Lane]

contrib/btree_gist declares type gbtreekey32 without any particular
alignment spec, which means it defaults to 4-byte ("integer") alignment.
However, btree_interval.c supposes that it can map a pair of intervals
onto that storage.  Since an interval contains a float8, that means we
end up trying to manipulate float8's that aren't double-aligned, causing
crashes on alignment-picky hardware.

I can replicate such a crash on my old HPPA box by doing this in
btree_gist's regression database (I'm just borrowing its interval
sample data):

contrib_regression=# create table ui (x text, a interval);
CREATE TABLE
contrib_regression=# insert into ui select null, a from intervaltmp ;
INSERT 0 612
contrib_regression=# insert into ui select '', a from intervaltmp ;
INSERT 0 612
contrib_regression=# insert into ui select 'q', a from intervaltmp ;
INSERT 0 612
contrib_regression=# insert into ui select 'qq', a from intervaltmp ;
INSERT 0 612
contrib_regression=# insert into ui select 'qqq', a from intervaltmp ;
INSERT 0 612
contrib_regression=# create index on ui using gist(x,a);
server closed the connection unexpectedly
 
The point of the text column of course is just to ensure that the index
contains gbtreekey32 entries that aren't maxaligned by virtue of
being at the front of their index tuple.

I failed to duplicate this crash on an old Apple PPC box, but it's
not 64-bit and I think the hardware is OK with doubles that are only
4-byte-aligned.  PPC64 would likely be less happy.

The same problem applies to mapping float8 or int8 onto gbtreekey16.

contrib_regression=# create table foo (x text, a int8);  
CREATE TABLE
contrib_regression=# insert into foo select null, a from int8tmp ; 
INSERT 0 600
contrib_regression=# insert into foo select '', a from int8tmp ;
INSERT 0 600
contrib_regression=# insert into foo select 'x', a from int8tmp ;
INSERT 0 600
contrib_regression=# insert into foo select 'xx', a from int8tmp ;
INSERT 0 600
contrib_regression=# insert into foo select 'xxx', a from int8tmp ;
INSERT 0 600
contrib_regression=# create index on foo using gist(x,a);
server closed the connection unexpectedly

We could fix this easily by redeclaring gbtreekey32 and gbtreekey16
with alignment = double, but that would cause issues for existing
multicolumn gist indexes (and I'm not sure how ALTER EXTENSION
UPDATE could manage such a change).  But the alternative of teaching
the code to avoid unaligned fetches seems messy and bug-prone.

Thoughts?

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: function xmltable

[Alvaro Herrera]

Pavel Stehule wrote:

> Here is updated patch without default namespace support (and without XPath
> expression transformation).
> 
> Due last changes in parser
> https://github.com/postgres/postgres/commit/906bfcad7ba7cb3863fe0e2a7810be8e3cd84fbd
> I had to use c_expr on other positions ( xmlnamespace definition).
> 
> I don't think it is limit - in 99% there will be const literal.

Argh.  I can't avoid the feeling that I'm missing some parser trickery
here.  We have the XMLNAMESPACES keyword and the clause-terminating
comma to protect these clauses, there must be a way to define this piece
of the grammar so that there's no conflict, without losing the freedom
in the expressions.  But I don't see how.  Now I agree that xml
namespace definitions are going to be string literals in almost all
cases (or in extra sophisticated cases, column names) ... it's probably
better to spend the bison-fu in the document expression or the column
options, or better yet the xmlexists_argument stuff.  But I don't see
possibility of improvements in any of those places, so let's put it
aside -- we can improve later, if need arises.

In any case, it looks like we can change c_expr to b_expr in a few
places, which is good because then operators work (in particular, unless
I misread the grammar, foo||bar doesn't work with c_expr and does work
with b_expr, which seems the most useful in this case).  Also, it makes
no sense to support (in the namespaces clause) DEFAULT a_expr if the
IDENT case uses only b_expr, so let's reduce both to just b_expr.

While I'm looking at node definitions, I see a few things that could use
some naming improvement.  For example, "expr" for TableExpr is a bit
unexpressive.  We could use "document_expr" there, perhaps.  "row_path"
seems fixated on the XML case and the expression be path; let's use
"row_expr" there.  And "cols" could be "column_exprs" perhaps.  (All
those renames cause fall-out in various node-related files, so let's
think carefully to avoid renaming them multiple times.)

In primnodes, you kept the comment that says "xpath".  Please update
that to not-just-XML reality.

Please fix the comment in XmlTableAddNs; NULL is no longer a valid value.

parse_expr.c has two unused variables; please remove them.

This test in ExecEvalTableExprProtected looks weird:
if (i != tstate->for_ordinality_col - 1)
please change to comparing "i + 1" (convert array index into attribute
number), and invert the boolean expression, leaving the for_ordinality
case on top and the rest in the "else".  That seems easier to read.
Also, we customarily use post-increment (rownum++) instead of pre-incr.

In execQual.c I think it's neater to have ExecEvalTableExpr go before
its subroutine.  Actually, I wonder whether it is really necessary to
have a subroutine in the first place; you could just move the entire
contents of that subroutine to within the PG_TRY block instead.  The
only thing you lose is one indentation level.  I'm not sure about this
one, but it's worth considering.

-- 
Á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] [BUG?] pg_event_trigger_ddl_commands() error with ALTER TEXT SEARCH CONFIGURATION

[Artur Zakirov]

2016-11-28 21:32 GMT+03:00 Robert Haas :
>
> You might need to add this patch to https://commitfest.postgresql.org/
> so that it doesn't get forgotten.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company

I added the patch to https://commitfest.postgresql.org/12/895/
I added it to the next commitfest. Because we are in the end of the
current commitfest.

-- 
Artur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company


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

Re: [HACKERS] GiST support for UUIDs

[Tom Lane]

Adam Brusselback  writes:
> [ btree_gist_uuid_7.patch ]

I spent awhile looking at this.  I have exactly no faith that it won't
crash on alignment-picky hardware, because this declaration:

union pg_uuid_t
{
unsigned char data[UUID_LEN];
uint64 v64[2];
};

is not the same as the existing pg_uuid_t; it instructs the compiler
that union pg_uuid_t must be double-aligned.  The existing type is
only byte-aligned, and is declared that way in pg_type, which means
that values on-disk are quite likely not to meet the alignment
expectation.

I spent a little bit of time trying to get the patch to crash on a PPC
machine, without success, but the compiler I have on that (gcc 4.0.1) is
very old and is not aggressive about things like optimizing memcpy calls
on supposedly-aligned arguments.  I think a modern gcc version on such
hardware would probably generate code that fails.  (Also, PPC is
big-endian, so some of the at-risk code isn't used; a picky little-endian
machine would have more scope for crashing.  Not real sure, but I think
ARM might be like that.)

What I would suggest is that you forget the union hack and just use
memcmp in all the comparison functions.  It's not likely to be worth
the trouble to try to get those calls to be safely aligned.  The
only place where you need go to any trouble is in uuid_parts_distance,
which could be redesigned to memcpy a not-necessarily-aligned source
into a local uint64[2] array and then byte-swap if needed.

(BTW, considering that we're going to return a float distance that has
only got twenty-something significant bits, do we *really* need to deal
with do-it-yourself int128 arithmetic in uuid_parts_distance?  Color
me skeptical.)

Also, I can't say that I think it's good style to be duplicating the
declaration of pg_uuid_t out of uuid.c.  (And it absolutely, positively,
is vile style to have two different declarations of the same struct in
different files, quite aside from whether they're ABI-compatible or not.)

I don't entirely see the point of making pg_uuid_t an opaque struct when
the only interesting thing about it, namely UUID_LEN, is exposed anyway.
So my inclination would be to just do

typedef struct pg_uuid_t
{
unsigned char data[UUID_LEN];
} pg_uuid_t;

in uuid.h and forget the idea of it being opaque.

Also, the patch could be made a good deal smaller (and easier to review)
in the wake of commit 40b449ae8: you no longer need to convert
btree_gist--1.2.sql into btree_gist--1.3.sql, just leave it alone and add
btree_gist--1.2--1.3.sql.  That way we don't have to worry about whether
the upgrade script matches the change in the base script.

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] PSQL commands: \quit_if, \quit_unless

[Andrew Dunstan]

On 11/28/2016 04:07 PM, Tom Lane wrote:

... We've generally refrained from inventing any control flow
metacommands for psql



I think it's really time we seriously considered adding some flow 
control logic, though. I'm mildly tired of either jumping through hoops 
to get around the lack or having to switch to using some other thing 
that has the required logic mechanism (hello Perl).


cheers

andrew


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

Re: [HACKERS] Tackling JsonPath support

[Petr Jelinek]

On 28/11/16 18:57, Christian Convey wrote:
> 
> On Mon, Nov 28, 2016 at 9:47 AM, Pavel Stehule  > wrote
> 
> > I thought by adding my first implementation to "contrib", we could make 
> this functionality available to end-users, even before there was a consensus 
> about what PG's "official" JSON-related operators should have for syntax and 
> semantics.
> >
> 
> this time the commiters dislike the contrib dir. It is hard to push
> there anything :-(. You can try it, but it can be lost time.
> 
> 
> ​Thanks for the warning.  I'm okay with  my patch adding the "json_path"
> function to the core PG code.​
> 
> I would still suggest that we hold off on having my first patch
> implement an official JSON-related operator such as "JSON_TABLE".  I
> would prefer to have my "json_path" function available to users even
> before we know how "JSON_TABLE", etc. should behave.
> 
> ​Does that sound reasonable?​
> 

Hi,

just make it extension, not contrib module, there is not much difference
between those except contrib is included in distribution.

Extensions that provide just functions are easy to integrate into core
(that's how some of the existing json functions were added in the past
as well).

-- 
  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] PSQL commands: \quit_if, \quit_unless

[Corey Huinker]

>
>
> As far as the original problem goes, I wonder whether what you really
>> want isn't a \quit command that lets you specify psql's exit code.
>>
>
>
The ability to specify an exit code was part of the brainstorming, yes. But
with it was the ability to conditionally quit.


> Actually, I'm seeing this as basically an assertion capability and maybe
> should be named as such
>
> \assert_is
> \assert_isnot ​
>

That came up too! I see real value in the ability to test for error
conditions. I just had a more immediate need for a non-error exit condition.

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

[David G. Johnston]

On Mon, Nov 28, 2016 at 2:07 PM, Tom Lane  wrote:

> Corey Huinker  writes:
> > On Mon, Nov 28, 2016 at 2:03 PM, Fabien COELHO 
> wrote:
>
There's no reason to assume a-priori that this patch creates either naming
> conventions or semantics (e.g. what is suitable as a boolean expression)
> that we'd be happy with in a larger context.
>
>
Would we be happy borrowing the definition of FOUND from pl/pgsql?​

As far as the original problem goes, I wonder whether what you really
> want isn't a \quit command that lets you specify psql's exit code.
>

Actually, I'm seeing this as basically an assertion capability and maybe
should be named as such

\assert_is
\assert_isnot ​

​David J.
​

Re: [HACKERS] Time to up bgwriter_lru_maxpages?

[Jim Nasby]

On 11/28/16 11:53 AM, Joshua D. Drake wrote:

On 11/28/2016 11:40 AM, Jim Nasby wrote:

With current limits, the most bgwriter can do (with 8k pages) is 1000
pages * 100 times/sec = 780MB/s. It's not hard to exceed that with
modern hardware. Should we increase the limit on bgwriter_lru_maxpages?


Considering a single SSD can do 70% of that limit, I would say yes.


Next question becomes... should there even be an upper limit?
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


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

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

[Tom Lane]

Corey Huinker  writes:
> On Mon, Nov 28, 2016 at 2:03 PM, Fabien COELHO  wrote:
>> I'm wondering if an simplistic interpreted \if \elsif/\else \fi would make
>> more sense:

> The problem is that \if \elsif \else \fi is anything but simplistic, and
> would be a vastly bigger change. Detecting nested if-thens, detecting
> un-matched if-thens, etc.

Yes, but...  We've generally refrained from inventing any control flow
metacommands for psql, and TBH this does *not* seem like the place to
start.  If we take this patch we are very likely to find that it doesn't
fit in at all whenever someone does show up with a proposal for control
flow features.

If we had an agreed-on sketch for what that feature set would look like,
I wouldn't necessarily object to implementing commands like these first.
But as it stands I think we'd be painting ourselves into a corner.
There's no reason to assume a-priori that this patch creates either naming
conventions or semantics (e.g. what is suitable as a boolean expression)
that we'd be happy with in a larger context.

As far as the original problem goes, I wonder whether what you really
want isn't a \quit command that lets you specify psql's exit code.
I'm not quite seeing how this proposal advances the problem of
communicating between psql and shell portions of a script; but I can
see how returning 0 (ok) vs 1 (error) vs 2 (some special case) would
help with that.

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] PSQL commands: \quit_if, \quit_unless

[Corey Huinker]

On Mon, Nov 28, 2016 at 2:03 PM, Fabien COELHO  wrote:

>
> Hello Corey,
>
> This patch adds two very simple psql commands: \quit_if and \quit_unless.
>>
>
> A few comments about the feature design:
>
> I'm unsure about the name, esp with '_'. There are some \lo_* commands,
> but others rely on pasted words (\crosstabview, \errverbose, ...).
>

I'm completely flexible with regard to the names.


> I'm wondering if an simplistic interpreted \if \elsif/\else \fi would make
> more sense:
>

The problem is that \if \elsif \else \fi is anything but simplistic, and
would be a vastly bigger change. Detecting nested if-thens, detecting
un-matched if-thens, etc.


> Quitting seems a little bit definitive, and means that if I have some
> alternatives then I have to have something that relaunch another script
> outside...
>

> When \includes are process, does \quit stop the include or the full
> script. I'm afraid it is the script.
>

It behaves exactly as if the script contained \quit at that location.


> Now probably an \if... would have also some drawbacks, but ISTM that there
> could be less of them.
>

It'd be nice, but I'm trying to keep things simple.


>
> There is no test provided with the patch.


Indeed, but testing such a feature is hard within our current test harness.
I welcome suggestions for how to convert the example script in my first
email to tests.

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

[Pavel Stehule]

2016-11-28 20:03 GMT+01:00 Fabien COELHO :

>
> Hello Corey,
>
> This patch adds two very simple psql commands: \quit_if and \quit_unless.
>>
>
> A few comments about the feature design:
>
> I'm unsure about the name, esp with '_'. There are some \lo_* commands,
> but others rely on pasted words (\crosstabview, \errverbose, ...).
>

There are not any other conditional statements - so using "_" can be better


>
> I'm wondering if an simplistic interpreted \if \elsif/\else \fi would make
> more sense:
>

The \if \ese \elseif is not in contradiction with \quit_if - \if is more
generic, \quit_if is simple for implementation, one liner, and best readable

Pavel


>
> Quitting seems a little bit definitive, and means that if I have some
> alternatives then I have to have something that relaunch another script
> outside...
>
> When \includes are process, does \quit stop the include or the full
> script. I'm afraid it is the script.
>
> Now probably an \if... would have also some drawbacks, but ISTM that there
> could be less of them.
>
> There is no test provided with the patch.
>
> --
> 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] Patch to implement pg_current_logfile() function

[Gilles Darold]

Le 28/11/2016 à 18:54, Karl O. Pinc a écrit :
> Hi Gilles,
>
> On Sun, 27 Nov 2016 21:54:46 +0100
> Gilles Darold  wrote:
>
>> I've attached the v15 of the patch that applies your changes/fixes and
>> remove the call to strtok().
> I like the idea of replacing the strtok() call with sscanf()
> but I see problems.
>
> It won't work if log_filename begins with a space because
> (the docs say) that a space in the sscanf() format represents
> any amount of whitespace.
Right

> As written, there's a potential buffer overrun in log_format.
> You could fix this by making log_format as large as lbuffer,
> but this seems clunky.

Sorry, Isee that I forgot to apply the control in number of character
read by sscanf.

The problem about white space make me though that the use of sscanf is
not the right solution, I will rewrite this part but I can not do that
before the end of the week.


-- 
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org



-- 
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] Time to up bgwriter_lru_maxpages?

[Joshua D. Drake]

On 11/28/2016 11:40 AM, Jim Nasby wrote:

With current limits, the most bgwriter can do (with 8k pages) is 1000
pages * 100 times/sec = 780MB/s. It's not hard to exceed that with
modern hardware. Should we increase the limit on bgwriter_lru_maxpages?


Considering a single SSD can do 70% of that limit, I would say yes.

JD



--
Command Prompt, Inc.  http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.
Unless otherwise stated, opinions are my own.


--
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] Time to up bgwriter_lru_maxpages?

[Devrim Gündüz]

Hi,

On Mon, 2016-11-28 at 11:40 -0800, Jim Nasby wrote:
> With current limits, the most bgwriter can do (with 8k pages) is 1000 
> pages * 100 times/sec = 780MB/s. It's not hard to exceed that with 
> modern hardware. Should we increase the limit on bgwriter_lru_maxpages?

+1 for that. I've seen many cases that we need more than 1000.

Regards, 
-- 
Devrim Gündüz
EnterpriseDB: http://www.enterprisedb.com
PostgreSQL Danışmanı/Consultant, Red Hat Certified Engineer
Twitter: @DevrimGunduz , @DevrimGunduzTR


signature.asc
Description: This is a digitally signed message part

Re: [HACKERS] Wrong order of tests in findDependentObjects()

[Tom Lane]

I wrote:
> Jim Nasby  writes:
>> I can't think of any reason you'd want the current behavior.

> But I think fixing it to not recurse to extensions during temp namespace
> cleanup might not be very hard.  I'll take a look.

Here's a draft patch for that.  Rather than sticking yet another special
assumption into deleteWhatDependsOn, I thought it was time to get rid of
that function altogether in favor of selecting its few special behaviors
via flag bits for performDeletion.  So this adds PERFORM_DELETION_QUIETLY
and PERFORM_DELETION_SKIP_ORIGINAL flag bits to do that, plus a
PERFORM_DELETION_SKIP_EXTENSIONS bit that solves the problem at hand.
Treating this as a performDeletion flag bit also lets us disable extension
dropping in autovacuum's forced drop of temp tables, which would otherwise
be a nasty hole in the fix.

I'm not sure if this is a candidate for back-patching or not.  I think
what it's fixing is mostly a convenience thing, since extension scripts
that explicitly drop any temp objects they've created are not at risk,
and that would be good extension authoring practice anyway IMO.
If we do back-patch we'd need to figure out whether we want to preserve
deleteWhatDependsOn as a stub function in the back branches.  (I rather
doubt that any third party code is calling it, since it's so
special-purpose, but you never know ...)

Thoughts?

regards, tom lane

diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index f71d80f..a32cde3 100644
*** a/src/backend/catalog/dependency.c
--- b/src/backend/catalog/dependency.c
*** static const Oid object_classes[] = {
*** 168,173 
--- 168,174 
  
  
  static void findDependentObjects(const ObjectAddress *object,
+ 	 int objflags,
  	 int flags,
  	 ObjectAddressStack *stack,
  	 ObjectAddresses *targetObjects,
*** static void findDependentObjects(const O
*** 175,181 
  	 Relation *depRel);
  static void reportDependentObjects(const ObjectAddresses *targetObjects,
  	   DropBehavior behavior,
! 	   int msglevel,
  	   const ObjectAddress *origObject);
  static void deleteOneObject(const ObjectAddress *object,
  Relation *depRel, int32 flags);
--- 176,182 
  	 Relation *depRel);
  static void reportDependentObjects(const ObjectAddresses *targetObjects,
  	   DropBehavior behavior,
! 	   int flags,
  	   const ObjectAddress *origObject);
  static void deleteOneObject(const ObjectAddress *object,
  Relation *depRel, int32 flags);
*** deleteObjectsInList(ObjectAddresses *tar
*** 237,247 
  	}
  
  	/*
! 	 * Delete all the objects in the proper order.
  	 */
  	for (i = 0; i < targetObjects->numrefs; i++)
  	{
  		ObjectAddress *thisobj = targetObjects->refs + i;
  
  		deleteOneObject(thisobj, depRel, flags);
  	}
--- 238,254 
  	}
  
  	/*
! 	 * Delete all the objects in the proper order, except that if told to, we
! 	 * should skip the original object(s).
  	 */
  	for (i = 0; i < targetObjects->numrefs; i++)
  	{
  		ObjectAddress *thisobj = targetObjects->refs + i;
+ 		ObjectAddressExtra *thisextra = targetObjects->extras + i;
+ 
+ 		if ((flags & PERFORM_DELETION_SKIP_ORIGINAL) &&
+ 			(thisextra->flags & DEPFLAG_ORIGINAL))
+ 			continue;
  
  		deleteOneObject(thisobj, depRel, flags);
  	}
*** deleteObjectsInList(ObjectAddresses *tar
*** 255,270 
   * according to the dependency type.
   *
   * This is the outer control routine for all forms of DROP that drop objects
!  * that can participate in dependencies.  Note that the next two routines
!  * are variants on the same theme; if you change anything here you'll likely
!  * need to fix them too.
   *
!  * flags should include PERFORM_DELETION_INTERNAL when the drop operation is
!  * not the direct result of a user-initiated action.  For example, when a
!  * temporary schema is cleaned out so that a new backend can use it, or when
!  * a column default is dropped as an intermediate step while adding a new one,
!  * that's an internal operation.  On the other hand, when we drop something
!  * because the user issued a DROP statement against it, that's not internal.
   */
  void
  performDeletion(const ObjectAddress *object,
--- 262,293 
   * according to the dependency type.
   *
   * This is the outer control routine for all forms of DROP that drop objects
!  * that can participate in dependencies.  Note that performMultipleDeletions
!  * is a variant on the same theme; if you change anything here you'll likely
!  * need to fix that too.
   *
!  * Bits in the flags argument can include:
!  *
!  * PERFORM_DELETION_INTERNAL: indicates that the drop operation is not the
!  * direct result of a user-initiated action.  For example, when a temporary
!  * schema is cleaned out so that a new backend can use it, or when a column
!  * default is dropped as an intermediate step while adding a new one, that's
!  * 

[HACKERS] Time to up bgwriter_lru_maxpages?

[Jim Nasby]

With current limits, the most bgwriter can do (with 8k pages) is 1000 
pages * 100 times/sec = 780MB/s. It's not hard to exceed that with 
modern hardware. Should we increase the limit on bgwriter_lru_maxpages?

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
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] Tackling JsonPath support

[Nico Williams]

On Mon, Nov 28, 2016 at 05:56:41PM +0100, Pavel Stehule wrote:
> Dne 28. 11. 2016 17:26 napsal uživatel "David Fetter" :
> > There's another option we should also consider: jq
> > .  It's available under a
> > PostgreSQL-compatible license, and has had a LOT of work put into
> > correctness and performance.
> 
> we can use it for inspiration. but the syntax of this tool is little bit
> too complex and too original against Json path ... jsonpath is relative
> simple implementation of xpath to json
> 
> we have one proprietary syntax already, two is maybe too much :-)

jq is hardly proprietary :)

JSON Path is not expressive enough (last I looked) and can be mapped
onto jq if need be anyways.

libjq has a number of desirable features, mostly its immutable/COW data
structures.  In libjq data structures are only mutated when there's
only one reference to them, but libjq's jv API is built around
immutability, so jv values are always notionally immutable.  For
example, one writes:

  jv a = jv_array();

  a = jv_array_append(a, jv_true()); // `a' is notionally new, but since
 // it had only one reference, its
 // memory is reused

and similarly for objects.  One could instead write:

  jv a = jv_array_append(jv_array(), jv_true());
  
or

  jv a = JV_ARRAY(jv_true());

One of the nice things about libjv is that almost every function
consumes a reference of every jv value passed in, with very few
exceptions.  This simplifies memory management, or at least avoidance of
double-free and use-after-free (it can be harder to track down leaks
though, since tools like valgrind don't understand that jv_copy() call
sites can be like allocations).

Nico
-- 


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

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

[Fabien COELHO]

Hello Corey,


This patch adds two very simple psql commands: \quit_if and \quit_unless.


A few comments about the feature design:

I'm unsure about the name, esp with '_'. There are some \lo_* commands, 
but others rely on pasted words (\crosstabview, \errverbose, ...).


I'm wondering if an simplistic interpreted \if \elsif/\else \fi would make 
more sense:


Quitting seems a little bit definitive, and means that if I have some 
alternatives then I have to have something that relaunch another script 
outside...


When \includes are process, does \quit stop the include or the full 
script. I'm afraid it is the script.


Now probably an \if... would have also some drawbacks, but ISTM that there 
could be less of them.


There is no test provided with the patch.

--
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] Fix comment in build_simple_rel

[Alvaro Herrera]

Amit Langote wrote:
> Attached fixes reference in a comment to a non-existent function:
> 
> s/GetRelationInfo/get_relation_info/g

Thanks, pushed.  get_relation_info() itself had been neglected when this
responsibility was added onto it; I added an entry there too.

-- 
Á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] Dynamic shared memory areas

[Robert Haas]

On Wed, Nov 23, 2016 at 7:07 AM, Thomas Munro
 wrote:
> Those let you create an area in existing memory (in a DSM segment,
> traditional inherited shmem).  The in-place versions will stlll create
> DSM segments on demand as required, though I suppose if you wanted to
> prevent that you could with dsa_set_size_limit(area, size).  One
> complication is that of course the automatic detach feature doesn't
> work if you're in some random piece of memory.  I have exposed
> dsa_on_dsm_detach, so that there is a way to hook it up to the detach
> hook for a pre-existing DSM segment, but that's the caller's
> responibility.

shm_mq_attach() made the opposite decision about how to solve this
problem, and frankly I think that API is a lot more convenient: if the
first argument to shm_mq_attach() happens to be located inside of a
DSM, you can pass the DSM as the second argument and it registers the
on_dsm_detach() hook for you.  If not, you can pass NULL and deal with
it in some other way.  But this makes the common case very simple.

-- 
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] Tackling JsonPath support

[Nico Williams]

I wonder what it might take to integrate jq[1] (via libjq) with
PostgreSQL...  The internal representation of JSON data is bound to be
completely different, no doubt, but jq is a fantastic language, akin to
XPath and XSLT combined, but with nice syntax.

[1] https://stedolan.github.io/jq (note: jq is NOT JQuery)

Nico
-- 


-- 
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] Mail thread references in commits

[Andrew Dunstan]

On 11/28/2016 09:49 AM, Alvaro Herrera wrote:

Magnus Hagander wrote:


I don't really read perl enough to take it apart. But
http://git.kernel.org/cgit/git/git.git/tree/gitweb/gitweb.perl is the code
(we're probably on an older version). I'm guessing it's coming out of
format_log_line (
http://git.kernel.org/cgit/git/git.git/tree/gitweb/gitweb.perl#n2035). (the
version we have only has the part that looks for the hash).

I think if we do want to fork, we should be looking at git_print_log
http://git.kernel.org/cgit/git/git.git/tree/gitweb/gitweb.perl#n4580
There's a regexp match that looks for "https://; but only when preceded
with "link: " (which is a bit odd, isn't it?).


I wonder if it's worth forking gitweb to make it do explicitly what we want
for this -- that is recognize all the different kinds of things that would
be interesting here. But that fork should probably be done by somebody with
some more perl skills than me :)

I think changing message-id links to URLs would be veyr handy, but
before proposing a fork I would like to have a better idea of how much
effort it entails.





I should point out that the link as Tom put it in his commit message is 
working fine on every MUA that I have tried: Thunderbird (my usual MUA), 
the GMail web interface, and the native MUAs on my Android Tablet and my 
iPhone. So I'm pretty happy with that, and want to make sure that 
nothing we do to accomodate gitweb would interfere with it. From my POV 
the pattern Tom used in that commit is perfect.


If there's a lack of perl-fu to do it, I hope to be in a better position 
to help a bit with that in a couple of weeks.


cheers

andrew



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

Re: [HACKERS] [BUG?] pg_event_trigger_ddl_commands() error with ALTER TEXT SEARCH CONFIGURATION

[Robert Haas]

On Sun, Nov 27, 2016 at 1:59 PM, Artur Zakirov  wrote:
> Thank you for answers.
>
> 2016-11-19 21:28 GMT+03:00 Michael Paquier :
>> On Thu, Nov 17, 2016 at 1:17 PM, Alvaro Herrera
>>  wrote:
>>> It's a bug.  You're right that we need to handle the object class
>>> somewhere.  Perhaps I failed to realize that tsconfigs could get
>>> altered.
>>
>> It seems to me that the thing to be careful of here is how a new
>> OBJECT_TSCONFIGURATIONMAP should use ObjectAddress. It does not seem
>> that complicated, but it needs some work.
>> --
>> Michael
>
> After some investigation it seems to me that OBJECT_TSCONFIGURATIONMAP
> can't be added for pg_ts_config_map. Because this catalog hasn't Oids.
>
> But this bug can be easily fixed (patch attached). I think in
> AlterTSConfiguration() TSConfigRelationId should be used instead of
> TSConfigMapRelationId. Secondly, in ProcessUtilitySlow() we can use
> commandCollected = true. Because configuration entry is added in
> EventTriggerCollectAlterTSConfig() into
> currentEventTriggerState->commandList.
>
> This patch only fixes the bug. But I think I also can do a patch which
> will give pg_ts_config_map entries with
> pg_event_trigger_ddl_commands() if someone wants. It can be done using
> new entry in the CollectedCommandType structure maybe.

You might need to add this patch to https://commitfest.postgresql.org/
so that it doesn't get forgotten.

-- 
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] Autovacuum breakage from a734fd5d1

[Robert Haas]

On Mon, Nov 28, 2016 at 12:18 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> I don't believe we should be so scared of the possibility of a serious
>> bug that can't be found through any of the ways we normally test that
>> we aren't willing to fix problems we can readily foresee.  I grant
>> that there are some situations where fixing a problem might involve
>> enough risk that we shouldn't attempt it, but this is (or was) pretty
>> straightforward code patterned after existing logic, and I really see
>> no reason to believe that anything that was wrong with it couldn't
>> have been debugged easily enough.
>
> I'm astonished that you think that.  A problem here would be almost
> impossible to diagnose/reproduce, I should think, given that it would
> require at least two different failures (first a backend not cleaning up
> after itself, and then something going wrong in autovac's drop attempt).
> If you had reason to believe there was something broken there, you could
> certainly hack the system enough to force it through that code sequence,
> but that's basically not something that would ever happen in routine
> testing.  So my judgment is that the odds of bugs being introduced here
> and then making it to the field outweighs the potential benefit over the
> long run.  We have enough hard-to-test code already, we do not need to add
> more for hypothetical corner cases.

The easiest way to test this would be to just hack the system catalogs
to be invalid in some way.  I think that frying relnatts or
relpersistence would cause a relcache build failure, which is
basically the kind of thing I'm worried about here.  I've seen plenty
of cases where a system was basically working and the user was
basically happy despite some minor catalog corruption ... until that
"minor" catalog corruption broke an entire subsystem.

A good example is relnamespace.  We've still not eliminated all of the
cases where a DROP SCHEMA concurrent with a CREATE  can
result in an object that lives in a nonexistent schema.  So you end up
with this orphaned object that nobody really cares about (after all,
they dropped the schema on purpose) but it doesn't really matter
because everything still runs just fine.  And then, as recently
happened with an actual EnterpriseDB customer, somebody tries to run
pg_dump.  As it turns out, pg_dump fails outright in this situation.
And now suddenly the customer is calling support.  The bad catalog
entries themselves aren't really an issue, but when some other system
like pg_dump or autovacuum chokes on them and *fails completely*
instead of *failing only on the problematic objects* it amplifies the
problem from something that affects only an object that nobody really
cares about into a problem that has a major impact on the whole
system.

> I did think of another argument on your side of this, which is that
> if you imagine that there's a persistent failure to drop some temp table,
> that would effectively disable autovacuum in that database.  Which
> would be bad.  But we could address that at very minimal risk just by
> moving the drop loop to after the vacuuming loop.  I note that the case
> I'm afraid of, a bug in the error-catching logic, could also lead to
> autovacuum becoming entirely disabled if we keep the drops first.

I agree that moving the DROP loop after the other loop has some
appeal, but I see a couple of problems.  One is that, by itself, it
doesn't prevent the cascading-failure problem I mentioned above.  If
the system is close to wrapround and the pg_class scan finds the temp
table that is holding back xmin after the table that can't be dropped
because the catalog is corrupt, then you're back in the situation
where a busted table keeps the system from doing the right thing on an
un-busted table.  The second is that dropping a table doesn't call
vac_update_datfrozenxid(); if we drop a table before running vacuum
operations, the results of the drop will be reflected in any
subsequent datfrozenxid update that may occur.  If we drop it
afterwards, it won't.

Perhaps neither of those things are totally horrible but they're not
especially good, either.

-- 
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] Tackling JsonPath support

[Christian Convey]

On Mon, Nov 28, 2016 at 9:47 AM, Pavel Stehule 
wrote
>
> > I thought by adding my first implementation to "contrib", we could make
> this functionality available to end-users, even before there was a
> consensus about what PG's "official" JSON-related operators should have for
> syntax and semantics.
> >
>
> this time the commiters dislike the contrib dir. It is hard to push there
> anything :-(. You can try it, but it can be lost time.
>

​Thanks for the warning.  I'm okay with  my patch adding the "json_path"
function to the core PG code.​

I would still suggest that we hold off on having my first patch implement
an official JSON-related operator such as "JSON_TABLE".  I would prefer to
have my "json_path" function available to users even before we know how
"JSON_TABLE", etc. should behave.

​Does that sound reasonable?​

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

Hi Gilles,

On Sun, 27 Nov 2016 21:54:46 +0100
Gilles Darold  wrote:

> I've attached the v15 of the patch that applies your changes/fixes and
> remove the call to strtok().

I like the idea of replacing the strtok() call with sscanf()
but I see problems.

It won't work if log_filename begins with a space because
(the docs say) that a space in the sscanf() format represents
any amount of whitespace.

As written, there's a potential buffer overrun in log_format.
You could fix this by making log_format as large as lbuffer,
but this seems clunky.

Regards,


Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Tackling JsonPath support

[Christian Convey]

On Mon, Nov 28, 2016 at 9:40 AM, Pavel Stehule 
wrote:
​...​

> > ​Hi Pavel,
> >
> > Can you clarify what you meant?  I *think* you're saying:
> >
> > * It's not important for me to match the syntax/semantics of the
> json-path implementations found in MySQL / Oracle / DB2 / ​MS SQL Server,
> and
> >
>
> oh no. the syntax is important. But for start we can have a subset. For
> json table function .. json to relation mapping is important path
> expression. some other features like predicates
> are nice, but can be implemented later.
>
> Im sorry. My English is bad.
>
​Hi Pavel,

You're English is very good, actually.  I think the confusion arises from
me speaking in vague terms.  I apologize for that.  Allow me to be more
specific about what I'm proposing to do.

I propose adding to "contrib" a function with the following characteristics:

* Its signature is "json_path( jsonb from_json, string
json_path_expression) --> jsonb".

* The function will hopefully be a useful building block for PG's
implementation of "official" JSON operators such as "JSON_TABLE".  Once the
PG community agrees on what those operators' syntax/semantics should be.
​
* The function will hopefully be immediately useful to PG users who want
JSONPath -like operations on their "jsonb" objects.

- C

[HACKERS] PSQL commands: \quit_if, \quit_unless

[Corey Huinker]

This patch adds two very simple psql commands: \quit_if and \quit_unless.

Each takes an optional string parameter and evaluates it for truthiness via
ParseVariableBool().

If a true-ish value is passed to \quit_if, psql will behave as if the user
had input \quit.

\quit_unless will do nothing if the value given was true-ish, and will
\quit in any other circumstances.

Examples below show the behavior:

$ psql postgres
psql (10devel)
Type "help" for help.

# \quit_if
# \quit_unless
$ psql postgres
psql (10devel)
Type "help" for help.

# \quit_if f
# \quit_if 0
# \quit_if false
# \quit_if 2
unrecognized value "2" for "\quit_if"; assuming "on"
$ psql postgres
psql (10devel)
Type "help" for help.

# \quit_unless 2
unrecognized value "2" for "\quit_unless"; assuming "on"
# \quit_unless f
$


The need for this patch has arisen from several scripts I've written
recently that were about 97% psql and 3% bash or similar glue language. In
those scripts, there's often a test that says "there is no work to do here,
and that is not an error". I could engineer an optional SELECT 1/0 to
generate an error, but then I have to make the bash script determine
whether that was an error-error or a no-everything-is-ok-error. I also
don't want to wrap hundreds of lines of SQL inside python docstrings (thus
losing syntax highlighting, etc) just to handle one if/then.

Many thanks to Pavel Stehule for brainstorming this one with me.
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 2410bee..a9b0732 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2616,6 +2616,23 @@ lo_import 152801
 
   
 
+  
+\quit_if string 
+
+
+Quits the psql program if the string 
evaluates to true.
+
+
+  
+
+  
+\quit_unless string 
+
+
+Quits the psql program unless the string 
evaluates to true.
+
+
+  
 
   
 \qecho text [ ... ] 
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index a9a2fdb..7b46fbd 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -1251,6 +1251,29 @@ exec_command(const char *cmd,
else if (strcmp(cmd, "q") == 0 || strcmp(cmd, "quit") == 0)
status = PSQL_CMD_TERMINATE;
 
+   /* \quit_if */
+   else if (strcmp(cmd, "quit_if") == 0)
+   {
+   char   *opt = psql_scan_slash_option(scan_state,
+   
 OT_NORMAL, NULL, false);
+
+   if (opt)
+   if (ParseVariableBool(opt, "\\quit_if"))
+   status = PSQL_CMD_TERMINATE;
+   }
+
+   /* \quit_unless */
+   else if (strcmp(cmd, "quit_unless") == 0)
+   {
+   char   *opt = psql_scan_slash_option(scan_state,
+   
 OT_NORMAL, NULL, false);
+
+   if (!opt)
+   status = PSQL_CMD_TERMINATE;
+   else if (! ParseVariableBool(opt, "\\quit_unless"))
+   status = PSQL_CMD_TERMINATE;
+   }
+
/* reset(clear) the buffer */
else if (strcmp(cmd, "r") == 0 || strcmp(cmd, "reset") == 0)
{
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index a69c4dd..db29650 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -177,6 +177,8 @@ slashUsage(unsigned short int pager)
fprintf(output, _("  \\gexec execute query, then 
execute each value in its result\n"));
fprintf(output, _("  \\gset [PREFIX] execute query and store 
results in psql variables\n"));
fprintf(output, _("  \\q quit psql\n"));
+   fprintf(output, _("  \\quit_if [STRING]  quit psql if STRING 
evaluates to true\n"));
+   fprintf(output, _("  \\quit_unless [STRING]  quit psql if STRING does 
not evaluate to true\n"));
fprintf(output, _("  \\crosstabview [COLUMNS] execute query and display 
results in crosstab\n"));
fprintf(output, _("  \\watch [SEC]   execute query every SEC 
seconds\n"));
fprintf(output, "\n");

-- 
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] Tackling JsonPath support

[Christian Convey]

On Mon, Nov 28, 2016 at 5:20 AM, Pavel Stehule 
wrote:
...

> Incremental work is great idea - I like this this style. Instead contrib,
> you can use public repository on github. Minimally for first stage is
> better to live outside core - you are not restricted by PostgreSQL
> development process. When your code will be stabilized, then you can go to
> commitfest. I believe so we need good JSON support. The XML support helps
> to PostgreSQL lot of, JSON will be great too.
>

​Hi Pavel,

Thanks for the suggestion.

I am planning to use one of my own public github repos as the location for
my work.

I thought by adding my first implementation to "contrib", we could make
this functionality available to end-users, even before there was a
consensus about what PG's "official" JSON-related operators should have for
syntax and semantics.

Does my reasoning make sense?

- C

[HACKERS] GIN non-intrusive vacuum of posting tree

[Andrew Borodin]

Hi hackers!

Here is a patch with more concurrency-friendly vacuum of GIN.

===Short problem statement===
Currently GIN vacuum during cleaning posting tree can lock this tree
for a long time without real need.

===Problem statement===
Vacuum of posting tree (function ginVacuumPostingTree() in ginvacuum.c
) is doing two passes through posting tree:

1. ginVacuumPostingTreeLeaves() takes LockBufferForCleanup,
effectively excluding all inserts. Then it traverses down trough tree
taking exclusive lock, effectively excluding all reads. On leaf level
it calls ginVacuumPostingTreeLeaf(), which deletes all dead tuples. If
there are any empty page, root lock is not relaesed, it passes to
stage two.

Interim: vacuum_delay_point(), which can hand for a while, holding
CleanupLock on root.

2. If there are any empty pages, ginScanToDelete() scans through tree,
deleting empty lead pages, then deleting empty inner pages, if they
emerged after leaf page deletion.


===Proposed algorithm===
ginVacuumPostingTreeLeaves() takes SHARED locks on inner pages and
EXCLUSIVE locks on leaf pages. On leaf pages it calls
ginVacuumPostingTreeLeaf().

If ginVacuumPostingTreeLeaves() encounters subtree with all leafs
empty, then it takes LockBufferForCleanup() on page P pointing to
empty subtree. After taking lock on P, ginScanToDelete() is called for
P. For every parent of P we can guarantee that this procedure will not
be necessary: if P was empty subtree itself, we would pass this
procedure to parent (unless P is root, than behavior is effectively
equal to before-patched).


===Testing===
This patch solved a problem encountered by Evgeniy Efimkin and
Vladimir Borodin from Yandex.Mail.
They implemented testbed with GIN index

CREATE TABLE BOX (uid bigint,
  lids integer[] NOT NULL
  CHECK (array_ndims(lids) = 1));

CREATE OR REPLACE FUNCTION ulids(
i_uid bigint,
i_lids integer[]
) RETURNS bigint[] AS $$
SELECT array_agg((i_uid << 32) | lid)
  FROM unnest(i_lids) lid;
$$ LANGUAGE SQL IMMUTABLE STRICT;

CREATE INDEX i_box_uid_lids
ON box USING gin (ulids(uid, lids)) WITH (FASTUPDATE=OFF);

Executing by a pgbench following query

\setrandom uid 1 150
\setrandom lid 1 1000
\setrandom lid2 1 1000
\setrandom lid3 1 1000
BEGIN;
insert into box values(:uid,'{:lid,:lid2,:lid3}');
insert into box values(:uid,'{}');
END;

and eventually deleting some of data. This testbed showed VACUUM
holding inserts for up to tenths of seconds. They claim that proposed
patch made vacuum invisible in this test.

Evgeniy, Vladimir, if I missed something or you have something to add,
please join discussion.

===Known issues===
1. Probably, there exists better algorithms. I could not adapt B-tree
vacuum wizardy to GIN, but that does not mean it is impossible.

2. There may be more CleanUp locks taken. Under write-dense scenarios,
this can lead to longer vacuum, but it should not consume more
resources, just wait.

3. I have changed locks sequence during page deleteion. I think that
it is safe, but comments there were in fear of inserts (excluded by
CleanupLock). More details in a code of ginDeletePage().

4. ginVacuumPostingTreeLeaves() traverses children pages after release
of parent lock (event SHARED). This is safe if there is only one
vacuum at a time. Is there a reason to be afraid of concurrent
vacuums?


I will be happy if someone points me were is a best place to read
about B-tree vacuum process. Or if someone will explain me how it
works in a few words.
Dev process is here
https://github.com/x4m/postgres_g/pull/2

Thank you for reading, I'm looking forward to hear your thought on the matter.


Best regards, Andrey Borodin.
diff --git a/src/backend/access/gin/ginbtree.c 
b/src/backend/access/gin/ginbtree.c
index a0afec4..dc28c81 100644
--- a/src/backend/access/gin/ginbtree.c
+++ b/src/backend/access/gin/ginbtree.c
@@ -30,7 +30,7 @@ static void ginFinishSplit(GinBtree btree, GinBtreeStack 
*stack,
 /*
  * Lock buffer by needed method for search.
  */
-static int
+int
 ginTraverseLock(Buffer buffer, bool searchMode)
 {
Pagepage;
diff --git a/src/backend/access/gin/ginvacuum.c 
b/src/backend/access/gin/ginvacuum.c
index 2685a1c..bc54284 100644
--- a/src/backend/access/gin/ginvacuum.c
+++ b/src/backend/access/gin/ginvacuum.c
@@ -108,75 +108,17 @@ xlogVacuumPage(Relation index, Buffer buffer)
PageSetLSN(page, recptr);
 }
 
-static bool
-ginVacuumPostingTreeLeaves(GinVacuumState *gvs, BlockNumber blkno, bool 
isRoot, Buffer *rootBuffer)
-{
-   Buffer  buffer;
-   Pagepage;
-   boolhasVoidPage = FALSE;
-   MemoryContext oldCxt;
-
-   buffer = ReadBufferExtended(gvs->index, MAIN_FORKNUM, blkno,
-   RBM_NORMAL, 
gvs->strategy);
-   page = BufferGetPage(buffer);
-
-   /*
-* We should be sure that we don't concurrent with inserts, insert 
process
-* 

Re: [HACKERS] UNDO and in-place update

[Robert Haas]

On Sun, Nov 27, 2016 at 10:44 PM, Amit Kapila  wrote:
> On Mon, Nov 28, 2016 at 4:50 AM, Robert Haas  wrote:
>> Well, my original email did contain a discussion of the need for
>> delete-marking.  I said that you couldn't do in-place updates when
>> indexed columns were modified unless the index AMs had support for
>> delete-marking, which is the same point you are making here.
>
> Sorry, I had not read that part earlier, but now that I read it, I
> think there is a slight difference in what I am saying.   I thought
> along with delete-marking, we might need transaction visibility
> information in the index as well.

I think we need to avoid putting the visibility information in the
index because that will make the index much bigger.  It would be a
shame to get the visibility index out of the heap (as this design
does) only to be forced to add it to the index.  Note that,
percentage-wise, it's quite a bit worse to have visibility information
in the index than in the heap, because the index tuples are going to
be much narrower than the heap tuples, often just one column.  (The
cost of having this information in the heap can be amortized across
the whole width of the table.)

I don't have the whole design for the delete-marking stuff worked out
yet.  I'm thinking we could have a visibility map where the bit for
each page is 1 if the page certainly has no pending UNDO and 0 if it
might have some.  In addition, if a tuple is deleted or the indexed
column value is changed, we delete-mark the associated index entries.
If we later discover that the page has no current UNDO (i.e. is
all-visible) and the tuple at a given TID matches our index entry, we
can clear the delete-mark for that index entry.  So, an index-only
scan rechecks the heap if the tuples is delete-marked OR the
visibility-map bit for the page is not set; if neither is the case, it
can assume the heap tuple is visible.

Another option would be to get rid of the visibility map and rely only
on the delete-marks.  If we did that, then tuples would have to be
delete-marked when first inserted since they wouldn't be all-visible
until sometime after the commit of the inserting transaction.

> BTW, it is not completely clear
> whether you want a delete-marking system or you think we could do
> without that by avoiding in-place updates, it seems to me from what
> you have written that you are inclined towards having a delete-marking
> system.

Yes, that's my inclination.  I don't think it would be necessary to
have the delete-marking in order to produce a committable patch, but
the benefit of this approach would be much reduced without that.

>>  However,
>> I agree that the case where the indexed column gets set back to a
>> previous value while the old index tuple for that value still exists
>> is particularly tricky.  I think that what we need to do there is
>> avoid inserting an index entry for a given value/TID pair if an index
>> entry for that value/TID pair already exists.
>
> Are you saying this for indexes with a delete-marking system or for
> indexes without that or for both?

I'm saying that in any case in which you allow in-place update, you
have to make sure you don't get multiple entries pointing at the same
TID unless they have different values in the index tuple.

>> That's a little different from your analysis.  In your step-3
>> analysis, you say that an index scan for 2 will find the step-1 tuple,
>> but that's not really right.  The index scan will find the index tuple
>> which points to the whole chain of tuples, step-3 => step-2 => step-1,
>> and it will decide which heap tuple from that chain the user can see
>> based on the user's snapshot.
>
> I think the scan will not traverse the chain if it starts after
> step-3's commit and that's what I intend to say.

I don't see why that would be true.

>>  That's OK, but we're in real trouble if
>> step-3 inserted an additional index tuple pointing to that chain
>> rather than simply noting that one already exists.  If it adds an
>> additional one, then we'll visit two index tuples for that TID.  Each
>> time, the visibility information in UNDO will cause us to return the
>> correct tuple, but we've erred by returning it twice (once per index
>> entry) rather than just once.
>
> Why will scan traverse the UNDO chain if it starts after step-3 commit?

Why wouldn't it?  I think that if an index scan hits a page with an
UNDO chain, it always need to traverse the whole thing.

-- 
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] Tackling JsonPath support

[Christian Convey]

On Mon, Nov 28, 2016 at 5:20 AM, Pavel Stehule 
wrote:
​...​

> Con: "JSON path expression" is a recurring them in the *grammars* of
>> user-facing operators in [1], [2], [3], and [4].  But it doesn't
>> necessarily follow that the function implemented in Step 2 will provide
>> useful infrastructure for PG's eventual implementations of "JSON_TABLE",
>> etc.
>>
>
> We can implement subset only - our XPath based on libxml2 does it too. The
> good target is support of usual examples on the net.
>

​Hi Pavel,

Can you clarify what you meant?  I *think* you're saying:

* It's not important for me to match the syntax/semantics of the json-path
implementations found in MySQL / Oracle / DB2 / ​MS SQL Server, and

* Instead, I should just use examples / explanations on the web as my
guidance.

​Thanks,​
Christian

Re: [HACKERS] pg_dump / copy bugs with "big lines" ?

[Alvaro Herrera]

I just wrote:

> The big advantage of your v3 patch is that it can be backpatched without
> fear of breaking ABI, so I've struggled to maintain that property in my
> changes.  We can further tweak in HEAD-only; for example change the API
> to use Size instead of int.  I think that would be desirable, but let's
> not do it until we have backpatched this one.

One thing I just noticed while trying to backpatch this is that we can
do so only to 9.5, because older branches do not have
MemoryContextAllocExtended().  They do have MemoryContextAllocHuge(),
but the caller in heaptuple.c wants zeroed memory too, so we'd need to
memset; I think that could get us back to 9.4.

9.3 and older is not possible because we don't have "huge alloc" there.

-- 
Á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] HASH_CHUNK_SIZE vs malloc rounding

[Tom Lane]

Thomas Munro  writes:
> I bet other allocators also do badly with "32KB plus a smidgen".  To
> minimise overhead we'd probably need to try to arrange for exactly
> 32KB (or some other power of 2 or at least factor of common page/chunk
> size?) to arrive into malloc, which means accounting for both
> nodeHash.c's header and aset.c's headers in nodeHash.c, which seems a
> bit horrible.  It may not be worth doing anything about.

Yeah, the other problem is that without a lot more knowledge of the
specific allocator, we shouldn't really assume that it's good or bad with
an exact-power-of-2 request --- it might well have its own overhead.
It is an issue though, and not only in nodeHash.c.  I'm pretty sure that
StringInfo also makes exact-power-of-2 requests for no essential reason,
and there are probably many other places.

We could imagine providing an mmgr API function along the lines of "adjust
this request size to the nearest thing that can be allocated efficiently".
That would avoid the need for callers to know about aset.c overhead
explicitly.  I'm not sure how it could deal with platform-specific malloc
vagaries though :-(

regards, tom lane


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

Re: [HACKERS] Autovacuum breakage from a734fd5d1

[Tom Lane]

Robert Haas  writes:
> I don't believe we should be so scared of the possibility of a serious
> bug that can't be found through any of the ways we normally test that
> we aren't willing to fix problems we can readily foresee.  I grant
> that there are some situations where fixing a problem might involve
> enough risk that we shouldn't attempt it, but this is (or was) pretty
> straightforward code patterned after existing logic, and I really see
> no reason to believe that anything that was wrong with it couldn't
> have been debugged easily enough.

I'm astonished that you think that.  A problem here would be almost
impossible to diagnose/reproduce, I should think, given that it would
require at least two different failures (first a backend not cleaning up
after itself, and then something going wrong in autovac's drop attempt).
If you had reason to believe there was something broken there, you could
certainly hack the system enough to force it through that code sequence,
but that's basically not something that would ever happen in routine
testing.  So my judgment is that the odds of bugs being introduced here
and then making it to the field outweighs the potential benefit over the
long run.  We have enough hard-to-test code already, we do not need to add
more for hypothetical corner cases.

I did think of another argument on your side of this, which is that
if you imagine that there's a persistent failure to drop some temp table,
that would effectively disable autovacuum in that database.  Which
would be bad.  But we could address that at very minimal risk just by
moving the drop loop to after the vacuuming loop.  I note that the case
I'm afraid of, a bug in the error-catching logic, could also lead to
autovacuum becoming entirely disabled if we keep the drops first.

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] Wrong order of tests in findDependentObjects()

[Tom Lane]

Jim Nasby  writes:
> On 11/27/16 10:15 AM, Tom Lane wrote:
>> Yeah, I was wondering about that yesterday --- that comment mentions
>> the case of temporary objects, but it only fixes the problem while the
>> script runs.  Maybe there should be a separate test for "we're doing
>> temporary-object cleanup" that would similarly prevent recursion to
>> an extension?

> I can't think of any reason you'd want the current behavior.

> Though, it'd arguably be better to remove temp objects created by an 
> extension after the script exits, so that they can't "leak" into the 
> executing backend. Dunno if that's any harder or not...

Sounds way harder to me.  There's no good way to separate temp objects
made by the script from those made earlier in the session.  Also, the
general theory of extension scripts is that they're just executed
normally, with the only additional bit of magic being that objects
created during the script are tied to the extension.  I'm not sure that
forcibly dropping temp objects at the end fits in that charter at all.

But I think fixing it to not recurse to extensions during temp namespace
cleanup might not be very hard.  I'll take a look.

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_dump / copy bugs with "big lines" ?

[Alvaro Herrera]

Daniel Verite wrote:

> And in enlargeStringInfo() the patch adds this:
>   /*
>* Maximum size for strings with allow_long=true.
>* It must not exceed INT_MAX, as the StringInfo routines
>* expect offsets into the buffer to fit into an int.
>*/
>   const int max_alloc_long = 0x7fff;
> 
> On a 32-bit arch, we can expect max_alloc_long == MaxAllocHugeSize,
> but on a 64-bit arch, it will be much smaller with MaxAllocHugeSize
> at (2^63)-1.

Yeah, you're right.  Here's a v4 of this patch, in which I've done some
further very small adjustments to your v3:

* I changed the 0x7fff hardcoded value with some arithmetic which
sholud have the same result on most architectures:

/*
 * Determine the upper size limit.  The fact that the StringInfo API 
uses
 * "int" everywhere limits us to int's width even for "long_ok" strings.
 */
limit = str->long_ok ?
(((Size) 1) << (Min(sizeof(int), sizeof(Size)) * 8 - 1)) - 1 :
MaxAllocSize;

Initially I just had "sizeof(int)" instead of the Min(), and a
StaticAssert for sizeof(int) <= sizeof(Size), on the grounds that no
actual platform is likely to break that (though I think the C standard
does allow it).  But I realized that doing it this way is simple enough;
and furthermore, in any platforms where int is 8 bytes (ILP64), this
would automatically allow for 63-bit-sized stringinfos.  I don't think
this exists today, but wikipedia[1] lists two obsolete ones, [2] and [3].

[1] https://en.wikipedia.org/wiki/64-bit_computing#64-bit_data_models
[2] https://en.wikipedia.org/wiki/HAL_SPARC64
[3] https://en.wikipedia.org/wiki/UNICOS

* I reverted the enlargement looping logic in enlargeStringInfo() to
pretty much the original code (minus the cast).

* I tweaked a few comments.

The big advantage of your v3 patch is that it can be backpatched without
fear of breaking ABI, so I've struggled to maintain that property in my
changes.  We can further tweak in HEAD-only; for example change the API
to use Size instead of int.  I think that would be desirable, but let's
not do it until we have backpatched this one.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/access/common/heaptuple.c 
b/src/backend/access/common/heaptuple.c
index e27ec78..631e555 100644
--- a/src/backend/access/common/heaptuple.c
+++ b/src/backend/access/common/heaptuple.c
@@ -741,7 +741,9 @@ heap_form_tuple(TupleDesc tupleDescriptor,
 * Allocate and zero the space needed.  Note that the tuple body and
 * HeapTupleData management structure are allocated in one chunk.
 */
-   tuple = (HeapTuple) palloc0(HEAPTUPLESIZE + len);
+   tuple = MemoryContextAllocExtended(CurrentMemoryContext,
+  
HEAPTUPLESIZE + len,
+  
MCXT_ALLOC_HUGE | MCXT_ALLOC_ZERO);
tuple->t_data = td = (HeapTupleHeader) ((char *) tuple + HEAPTUPLESIZE);
 
/*
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 3c81906..ec5d6f1 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -385,7 +385,7 @@ ReceiveCopyBegin(CopyState cstate)
pq_sendint(, format, 2);/* per-column 
formats */
pq_endmessage();
cstate->copy_dest = COPY_NEW_FE;
-   cstate->fe_msgbuf = makeStringInfo();
+   cstate->fe_msgbuf = makeLongStringInfo();
}
else
{
@@ -1907,7 +1907,7 @@ CopyTo(CopyState cstate)
cstate->null_print_client = cstate->null_print; /* default */
 
/* We use fe_msgbuf as a per-row buffer regardless of copy_dest */
-   cstate->fe_msgbuf = makeStringInfo();
+   cstate->fe_msgbuf = makeLongStringInfo();
 
/* Get info about the columns we need to process. */
cstate->out_functions = (FmgrInfo *) palloc(num_phys_attrs * 
sizeof(FmgrInfo));
@@ -2742,8 +2742,8 @@ BeginCopyFrom(ParseState *pstate,
cstate->cur_attval = NULL;
 
/* Set up variables to avoid per-attribute overhead. */
-   initStringInfo(>attribute_buf);
-   initStringInfo(>line_buf);
+   initLongStringInfo(>attribute_buf);
+   initLongStringInfo(>line_buf);
cstate->line_buf_converted = false;
cstate->raw_buf = (char *) palloc(RAW_BUF_SIZE + 1);
cstate->raw_buf_index = cstate->raw_buf_len = 0;
diff --git a/src/backend/lib/stringinfo.c b/src/backend/lib/stringinfo.c
index 7382e08..35f2eab 100644
--- a/src/backend/lib/stringinfo.c
+++ b/src/backend/lib/stringinfo.c
@@ -37,10 +37,28 @@ makeStringInfo(void)
 }
 
 /*
+ * makeLongStringInfo
+ *
+ * Same as makeStringInfo for larger strings.
+ */
+StringInfo
+makeLongStringInfo(void)
+{
+   

Re: [HACKERS] Tackling JsonPath support

[Pavel Stehule]

Dne 28. 11. 2016 17:26 napsal uživatel "David Fetter" :
>
> On Sun, Nov 27, 2016 at 11:50:30AM -0500, Christian Convey wrote:
> > >From looking at other databases' docs, it seems like the behavior of
> > various JSON-related operators / functions are described partially in
terms
> > of a "json path expression":
> >
> > * In Oracle, "JSON_TABLE", "JSON_exists_column", "JSON_value_column":
[1]
> > * In MySQL: [2]
> > * In DB2: [3]
> > * In MS SQL Server: [4]
> > * (Whatever the Standards committee will end up producing.)
>
> There's another option we should also consider: jq
> .  It's available under a
> PostgreSQL-compatible license, and has had a LOT of work put into
> correctness and performance.

we can use it for inspiration. but the syntax of this tool is little bit
too complex and too original against Json path ... jsonpath is relative
simple implementation of xpath to json

we have one proprietary syntax already, two is maybe too much :-)

>
> 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

Re: [HACKERS] A bug of psql completion

[Tom Lane]

Kyotaro HORIGUCHI  writes:
> I noticed that the psql completion code for "ALTER TABLE x ALTER
> [COLUMN] x DROP" is wrong. It works as the following

> =# alter table x alter x drop 
> [nothing suggested]
> =# alter table x table x alter x drop 
> DEFAULT   NOT NULL  

> The attached patch fixes it.

Pushed, thanks.

regards, tom lane


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

Re: [HACKERS] Alternative MATERIALIZED VIEW design and implementation with history table and other features

[Nico Williams]

On Sat, Nov 26, 2016 at 04:13:19PM -0600, Jim Nasby wrote:
> On 11/22/16 9:11 PM, Nico Williams wrote:
> >But we needed a method for recording deltas from REFRESHes, and that's
> >not supported.  So I coded up my own version of materialized views, in
> >PlPgSQL, that does provide a history feature.
> 
> Getting history tracking included in core is going to take a LOT of effort;
> not from a code standpoint, but to get everyone to agree on the use cases,
> interface, etc. In short: I wouldn't go there.

Thanks for the reply.

Well, I want to go there :)  And, frankly, I think the interface details
aren't that hard, though I could be wrong.

The bare minimum[0] columns for the history table are: a transaction ID,
a column of the view's record type[1] for "old"/deleted rows, and a
column of the view's record type for "new"/inserted rows.

If a row was deleted, then you get a row in the history table with that
row's record value as the "old" column's value, with the "new" column's
value being NULL.  Similarly for insertions.  For updates, if there's a
primary key[2] we could have a single history row with non-NULL values
for all three columns, else two history rows, one with the old row as
the "old" column's value and NULL for the "new" column, and another
history row with a NULL for the "old" column and the new row as the
value of the "new" column.

The trickiest thing here is the "transaction ID".  In my use case I have
to deal with an ORM's notion of transaction ID, so no PG notion of TX ID
helps me :(

> What *would* be useful is work on generating delta information from a set of
> changes to a table: see below.

My implementation does that too (via triggers).

> >Besides a history feature, this includes the ability to record changes
> >made to a materialized view's materialization table, which means I can
> >have triggers that update the materialized view.
> 
> Just looking at the commend block in the file, it's not clear what you mean
> by this. Does this mean if you ALTER the "materialization table" (which I
> assume is the materialized output?), does the view somehow get modified?

It means that if you INSERT/UPDATE/DELETE on a VIEW materialized with my
implementation, then those changes are recorded in the history table for
the view.  However, it is important that such changes be consistent with
the VIEW, else the next refresh will undo them (naturally).

> >Of particular interest may be the fact that the FULL OUTER JOIN that PG
> >does for REFRESH CONCURRENTLY, and which I copied here, doesn't deal
> >well with views that have NULLs in any columns used in the join.  It
> >would be nice to have an equijoin that uses IS NOT DISTINCT FROM rather
> >than just =, and then refreshing could use such a join in order to deal
> >properly with NULLs.
> 
> Well, you could potentially just cast the composite types to text and join
> on that, but...

Only if there's a non-ambiguous text representation of row values.
Also, this might make refreshes slow.

> Kevin Grittner (author of the original matviews patch) has mentioned
> detecting changes to underlying relations previously[1]. Getting even a
> simple form of that working is going to be critical for any kind of
> incremental matview updating. It sounds like you're doing something
> different from incremental update, but the core problem is still the same:
> how to efficiently identify changes to underlying matview data and apply
> those changes to the view.

Incremental update is *exactly* what I'm doing.  It's also exactly what
REFRESH CONCURRENTLY does today, except that REFRESH CONCURRENTLY
doesn't save the history but my implementation does.

> I suggest taking a look at what Kevin's written about that, as well as the

Sure.

> paper he mentioned. Some of this does assume that you have the equivalent to
> NEW and OLD, but IIRC those are not actually mandatory (ie: you could use
> the current matview contents as OLD and the dynamic view as NEW). Even a

My implementation is rather trivial.  You might want to look at it.  Its
guts are actually copied from what PG already does for REFRESH
CONCURRENTLY.

> pure SQL/plpgsql implementation of what's in the paper that Kevin mentioned
> would probably be valuable to ongoing work, as well as being applicable to
> what you've done.
> 
> 1: 
> https://www.postgresql.org/message-id/1371480075.55528.yahoomail...@web162901.mail.bf1.yahoo.com

Thanks for the link.

My implementation even supports auto-refresh of stale VIEWs and uses
NOTIFY/LISTEN channels as the FIFO.  There is a PGSQL-coded conditional
refresh function (refresh_if_needed()) that a daemon calls periodically
and whenever it gets a NOTIFY on a channel that it LISTENs on; the
"sla", last refresh timestamp, and "refresh needed" indicators for each
materialized view, are recorded in a table.

That's what the pqasyncnotifier.c file is about: it makes it possible[3]
to shell-code a daemon that refreshes stale materialized views
automatically.  The daemon for 

Re: [HACKERS] Tackling JsonPath support

[David Fetter]

On Sun, Nov 27, 2016 at 11:50:30AM -0500, Christian Convey wrote:
> >From looking at other databases' docs, it seems like the behavior of
> various JSON-related operators / functions are described partially in terms
> of a "json path expression":
> 
> * In Oracle, "JSON_TABLE", "JSON_exists_column", "JSON_value_column": [1]
> * In MySQL: [2]
> * In DB2: [3]
> * In MS SQL Server: [4]
> * (Whatever the Standards committee will end up producing.)

There's another option we should also consider: jq
.  It's available under a
PostgreSQL-compatible license, and has had a LOT of work put into
correctness and performance.

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] pgpassfile connection option

[Fabien COELHO]

Hello Julian,


I've adressed those spacing errors.


Ok.

You are right, if pgpassfile_used is true, it SHOULD be defined, I just like 
to be careful whenever I'm working with strings. But I guess in this scenario 
I can trust the caller and omit those checks.


Good.

Patch looks ok, applies, compiles & checks, and tested manually.

I've switch in the CF to "ready for committer", and we'll see what the 
next level thinks about it:-)


[...] I agree with those criticisms of the multi-host feature and 
notifying the client in case of an authentification error rather than 
trying other hosts seems sensible to me.


Sure. I complained about the fuzzy documentation & imprecise warning 
message because I stumbled upon that while testing.


But I think fixes for those should be part of different patches, as this 
patch's aim was only to expand the existing pgpassfile functionality to 
be used with a parameter.


Yes.

--
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] [PATCH] ALTER DEFAULT PRIVILEGES with GRANT/REVOKE ON SCHEMAS

[Matheus de Oliveira]

Just sending the same patch but rebase with current master (it was broken
for gram.y after new commits).

Best regards,
diff --git a/doc/src/sgml/ref/alter_default_privileges.sgml b/doc/src/sgml/ref/alter_default_privileges.sgml
index 04064d3..7745792 100644
*** a/doc/src/sgml/ref/alter_default_privileges.sgml
--- b/doc/src/sgml/ref/alter_default_privileges.sgml
***
*** 46,51  GRANT { USAGE | ALL [ PRIVILEGES ] }
--- 46,55 
  ON TYPES
  TO { [ GROUP ] role_name | PUBLIC } [, ...] [ WITH GRANT OPTION ]
  
+ GRANT { USAGE | CREATE | ALL [ PRIVILEGES ] }
+ ON SCHEMAS
+ TO { [ GROUP ] role_name | PUBLIC } [, ...] [ WITH GRANT OPTION ]
+ 
  REVOKE [ GRANT OPTION FOR ]
  { { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER }
  [, ...] | ALL [ PRIVILEGES ] }
***
*** 71,76  REVOKE [ GRANT OPTION FOR ]
--- 75,86 
  ON TYPES
  FROM { [ GROUP ] role_name | PUBLIC } [, ...]
  [ CASCADE | RESTRICT ]
+ 
+ REVOKE [ GRANT OPTION FOR ]
+ { USAGE | CREATE | ALL [ PRIVILEGES ] }
+ ON SCHEMAS
+ FROM { [ GROUP ] role_name | PUBLIC } [, ...]
+ [ CASCADE | RESTRICT ]
  
   
  
***
*** 125,130  REVOKE [ GRANT OPTION FOR ]
--- 135,142 
are altered for objects later created in that schema.
If IN SCHEMA is omitted, the global default privileges
are altered.
+   IN SCHEMA is not allowed when using ON SCHEMAS
+   as schemas can't be nested.
   
  
 
diff --git a/src/backend/catalog/aclchk.c b/src/backeindex c0df671..e1256f1 100644
*** a/src/backend/catalog/aclchk.c
--- b/src/backend/catalog/aclchk.c
***
*** 948,953  ExecAlterDefaultPrivilegesStmt(ParseState *pstate, AlterDefaultPrivilegesStmt *s
--- 948,957 
  			all_privileges = ACL_ALL_RIGHTS_TYPE;
  			errormsg = gettext_noop("invalid privilege type %s for type");
  			break;
+ 		case ACL_OBJECT_NAMESPACE:
+ 			all_privileges = ACL_ALL_RIGHTS_NAMESPACE;
+ 			errormsg = gettext_noop("invalid privilege type %s for schema");
+ 			break;
  		default:
  			elog(ERROR, "unrecognized GrantStmt.objtype: %d",
   (int) action->objtype);
***
*** 1135,1140  SetDefaultACL(InternalDefaultACL *iacls)
--- 1139,1154 
  this_privileges = ACL_ALL_RIGHTS_TYPE;
  			break;
  
+ 		case ACL_OBJECT_NAMESPACE:
+ 			if (OidIsValid(iacls->nspid))
+ ereport(ERROR,
+ 		(errcode(ERRCODE_INVALID_GRANT_OPERATION),
+ 		 errmsg("cannot use IN SCHEMA clause when using GRANT/REVOKE ON SCHEMAS")));
+ 			objtype = DEFACLOBJ_NAMESPACE;
+ 			if (iacls->all_privs && this_privileges == ACL_NO_RIGHTS)
+ this_privileges = ACL_ALL_RIGHTS_NAMESPACE;
+ 			break;
+ 
  		default:
  			elog(ERROR, "unrecognized objtype: %d",
   (int) iacls->objtype);
***
*** 1361,1366  RemoveRoleFromObjectACL(Oid roleid, Oid classid, Oid objid)
--- 1375,1383 
  			case DEFACLOBJ_TYPE:
  iacls.objtype = ACL_OBJECT_TYPE;
  break;
+ 			case DEFACLOBJ_NAMESPACE:
+ iacls.objtype = ACL_OBJECT_NAMESPACE;
+ break;
  			default:
  /* Shouldn't get here */
  elog(ERROR, "unexpected default ACL type: %d",
***
*** 5189,5194  get_user_default_acl(GrantObjectType objtype, Oid ownerId, Oid nsp_oid)
--- 5206,5215 
  			defaclobjtype = DEFACLOBJ_TYPE;
  			break;
  
+ 		case ACL_OBJECT_NAMESPACE:
+ 			defaclobjtype = DEFACLOBJ_NAMESPACE;
+ 			break;
+ 
  		default:
  			return NULL;
  	}
diff --git a/src/backend/catalog/obindex d531d17..23d6684 100644
*** a/src/backend/catalog/objectaddress.c
--- b/src/backend/catalog/objectaddress.c
***
*** 1788,1798  get_object_address_defacl(List *objname, List *objargs, bool missing_ok)
  		case DEFACLOBJ_TYPE:
  			objtype_str = "types";
  			break;
  		default:
  			ereport(ERROR,
  	(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
    errmsg("unrecognized default ACL object type %c", objtype),
! 	 errhint("Valid object types are \"r\", \"S\", \"f\", and \"T\".")));
  	}
  
  	/*
--- 1788,1801 
  		case DEFACLOBJ_TYPE:
  			objtype_str = "types";
  			break;
+ 		case DEFACLOBJ_NAMESPACE:
+ 			objtype_str = "schemas";
+ 			break;
  		default:
  			ereport(ERROR,
  	(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
    errmsg("unrecognized default ACL object type %c", objtype),
! 	 errhint("Valid object types are \"r\", \"S\", \"f\", \"T\" and \"s\".")));
  	}
  
  	/*
***
*** 3093,3098  getObjectDescription(const ObjectAddress *object)
--- 3096,3106 
  		 _("default privileges on new types belonging to role %s"),
  			   GetUserNameFromId(defacl->defaclrole, false));
  		break;
+ 	case DEFACLOBJ_NAMESPACE:
+ 		appendStringInfo(,
+ 		 _("default privileges on new schemas belonging to role %s"),
+ 			   GetUserNameFromId(defacl->defaclrole, false));
+ 		break;
  	default:
  		/* shouldn't get here */
  		appendStringInfo(,

Re: [HACKERS] Proposal: scan key push down to heap [WIP]

[Robert Haas]

On Mon, Nov 28, 2016 at 4:30 AM, Dilip Kumar  wrote:
> On Sat, Nov 19, 2016 at 6:48 PM, Dilip Kumar  wrote:
>> patch1: Original patch (heap_scankey_pushdown_v1.patch), only
>> supported for fixed length datatype and use heap_getattr.
>>
>> patch2: Switches memory context in HeapKeyTest + Store tuple in slot
>> and use slot_getattr instead of heap_getattr.
>>
>> patch3: call HeapKeyTest in SeqNext after storing slot, and also
>> switch memory context before calling HeapKeyTest.
>>
>> I haven't yet tested patch3 with TPCH, I will do that once machine is 
>> available.
>
> As promised, I have taken the performance with TPCH benchmark and
> still result are quite good. However this are less compared to older
> version (which was exposing expr ctx and slot to heap).
>
> Query   Head [1] Patch3   Improvement
> Q3   36122.425  32285.608 10%
> Q4   6797   5763.871   15%
> Q1017996.104  15878.505  11%
> Q1212399.6519969.489  19%
>
>  [1] heap_scankey_pushdown_POC_V3.patch : attached with the mail.

I think we should go with this approach.  I don't think it's a good
idea to have the heapam layer know about executor slots.  Even though
it's a little sad to pass up an opportunity for a larger performance
improvement, this improvement is still quite good.  However, there's a
fair amount of this patch that doesn't look right:

- The changes to heapam.c shouldn't be needed any more.  Ditto
valid.h, relscan.h, catcache.c and maybe some other stuff.

- get_scankey_from_qual() should be done at plan time, not execution
time.  Just as index scans already divide up quals between "Index
Cond" and "Filter" (see ExplainNode), I think Seq Scans are now going
to need to something similar.  Obviously, "Index Cond" isn't an
appropriate name for things that we test via HeapKeyTest, but maybe
"Heap Cond" would be suitable. That's going to be a fair amount of
refactoring, since the "typedef Scan SeqScan" in plannodes.h is going
to need to be replaced by an actual new structure definition.

- get_scankey_from_qual()'s prohibition on variable-width columns is
presumably no longer necessary with this approach, right?

- Anything tested in SeqNext() will also need to be retested in
SeqRecheck(); otherwise, the test will be erroneously skipped during
EPQ rechecks.

-- 
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] UNDO and in-place update

[Bruce Momjian]

On Sun, Nov 27, 2016 at 09:19:06AM +0530, Amit Kapila wrote:
> At this point, index scan for value 2 will find index tuple of step-1
> (2) and will conclude 2,def as a right tuple, but actually, that is
> wrong as the step-1 (2) index tuple should not be visible to the user.
> Do you also this as a problem or am I missing something?  If this a
> problem, then I think we need some form of delete marking system for
> the index, probably transaction visibility information as well.

Yes, very similar to the problems with WARM already discussed.

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

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


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

Re: [HACKERS] Mail thread references in commits

[Alvaro Herrera]

Magnus Hagander wrote:

> I don't really read perl enough to take it apart. But
> http://git.kernel.org/cgit/git/git.git/tree/gitweb/gitweb.perl is the code
> (we're probably on an older version). I'm guessing it's coming out of
> format_log_line (
> http://git.kernel.org/cgit/git/git.git/tree/gitweb/gitweb.perl#n2035). (the
> version we have only has the part that looks for the hash).

I think if we do want to fork, we should be looking at git_print_log
http://git.kernel.org/cgit/git/git.git/tree/gitweb/gitweb.perl#n4580
There's a regexp match that looks for "https://; but only when preceded
with "link: " (which is a bit odd, isn't it?).

> I wonder if it's worth forking gitweb to make it do explicitly what we want
> for this -- that is recognize all the different kinds of things that would
> be interesting here. But that fork should probably be done by somebody with
> some more perl skills than me :)

I think changing message-id links to URLs would be veyr handy, but
before proposing a fork I would like to have a better idea of how much
effort it entails.

-- 
Á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] Declarative partitioning - another take

[Ashutosh Bapat]

Here are some comments on 0003 patch.
1. In ALTER TABLE documentation we should refer to CREATE TABLE documentation
for partition_bound_spec syntax, similar ADD table_constraint note.

2. I think, "of the target table" is missing after all the ... constraints
+  match.  Also, it must have all the NOT NULL and
+  CHECK constraints present in the target table.

3. I think, using "any of the" instead of "some" is preferred in the following
sentence. In the following sentence, we should use "such a constraint" instead
of "constraints" to avoid mixed singular and plural usage.
+  If some CHECK constraint of the table being attached

4. This construction is ambiguous. "are not considered" - considered where? for
what? Constraints on which object?
+  clause.  Currently UNIQUE, PRIMARY
KEY,
+  and FOREIGN KEY constraints are not considered.

5. What is a partition constraint? Do we define that term anywhere in the
documentation? If not we should define that term and then use it here.
+  A full table scan is performed on the table being attached to check that
+  no existing row in the table violates the partition constraint.  It is

6. The paragraph following
+  A full table scan is performed on the table
being attached to check that seems to be confusing. It says that the
potentially expensive scan can be avoided by adding a constraint equivalent to
partition constraint. That seems to be wrong. The table will be scanned anyway,
when adding a valid constraint per ALTER TABLE ADD table_constraint
documentation. Instead you might want to say that the table scan will not be
carried out if the existing constraints imply the partition constraints.

7. You might want to specify the fate of range or lists covered by the
partition being detached in DETACH PARTITION section.

8. Since partition bound specification is more than a few words, you might want
to refer to the actual syntax in CREATE TABLE command.
+  partition_bound_spec
+  
+   
+The partition bound specification for a new partition.
+   
+  
+ 

9. I think, we need to use "may be scanned" instead of "is scanned", given that
some other part of the documentation talks about "avoiding" the table scan. May
be refer to that documentation to clarify the uncertainty.
+Similarly, when attaching a new partition it is scanned to verify that

10. The same paragraph talks about multiple ALTER TABLE commands being run
together to avoid multiple table rewrites. Do we want to document whether
ATTACH/DETACH partition can be run with any other command or not.

11. ATTACH partition documentation is not talking about the unlogged or
temporary tables being attached to is logged or non-temporary. What is current
code doing about these cases? Do we allow such mixed partition hierarchy?
+existing rows meet the partition constraint.

I will continue to look at the patch.

On Thu, Nov 24, 2016 at 4:04 PM, Amit Langote
 wrote:
>
> Hi Rushabh,
>
> On 2016/11/22 22:11, Rushabh Lathia wrote:
>> Hi Amit,
>>
>> I was just reading through your patches and here are some quick review
>> comments
>> for 0001-Catalog-and-DDL-for-partitioned-tables-17.patch.
>
> Thanks for the review!
>
>>
>> Review comments for  0001-Catalog-and-DDL-for-partitioned-tables-17.patch:
>>
>> 1)
>> @@ -1102,9 +1104,10 @@ heap_create_with_catalog(const char *relname,
>>  {
>>  /* Use binary-upgrade override for pg_class.oid/relfilenode? */
>>  if (IsBinaryUpgrade &&
>> -(relkind == RELKIND_RELATION || relkind == RELKIND_SEQUENCE ||
>> - relkind == RELKIND_VIEW || relkind == RELKIND_MATVIEW ||
>> - relkind == RELKIND_COMPOSITE_TYPE || relkind ==
>> RELKIND_FOREIGN_TABLE))
>> +(relkind == RELKIND_RELATION || relkind ==
>> RELKIND_PARTITIONED_TABLE ||
>> + relkind == RELKIND_SEQUENCE || relkind == RELKIND_VIEW ||
>> + relkind == RELKIND_MATVIEW || relkind ==
>> RELKIND_COMPOSITE_TYPE ||
>> + relkind == RELKIND_FOREIGN_TABLE))
>>
>> You should add the RELKIND_PARTITIONED_TABLE at the end of if condition that
>> will make diff minimal. While reading through the patch I noticed that there
>> is inconsistency - someplace its been added at the end and at few places its
>> at the start. I think you can make add it at the end of condition and be
>> consistent with each place.
>
> OK, done.
>
>>
>> 2)
>>
>> +/*
>> + * We need to transform the raw parsetrees corresponding to
>> partition
>> + * expressions into executable expression trees.  Like column
>> defaults
>> + * and CHECK constraints, we could not have done the transformation
>> + * earlier.
>> + */
>>
>>
>> Additional space before "Like column defaults".
>
> I think it's a common practice to add two spaces after a sentence-ending
> period [https://www.gnu.org/prep/standards/html_node/Comments.html], which
> it seems, is 

Re: [HACKERS] User-defined Operator Pushdown and Collations

[Paul Ramsey]

On Sun, Nov 27, 2016 at 11:50 AM, Tom Lane  wrote:

> Paul Ramsey  writes:
> > On Sun, Nov 27, 2016 at 9:31 AM, Tom Lane  wrote:
> >> Why doesn't hs_fdw.h have a collation?
>
> > I think I'm missing something, I cannot find a file like that anywhere.
>
> I was referring to the variable shown in your EXPLAIN.
>

Ah right. Why would hs_fdw.h have a collation, it's an hstore. It's not
declared with a collation (the CREATE TYPE call doesn't set the COLLATEABLE
attribue to true). Again my ignorance is running ahead of me: does every
object in the database necessarily have a collation?

CREATE FOREIGN TABLE hs_fdw ( id integer, h hstore collate "en_CA.UTF-8")
server foreign_server OPTIONS (table_name 'hs');
ERROR: collations are not supported by type hstore


> > With respect to this particular example, is this a case of a very large
> > collation hammer getting in the way? Both '->' and '=' are operators that
> > would be unaffected by collation, right? They are both just
> equality-based
> > tests. But all operators are getting tested for coherent collation
> > behaviour, so they get caught up in the net?
>
> Yeah, we don't know whether the operator actually cares about its input
> collation.  It'd be possible to be much more liberal if we knew it did
> not, but such labeling was not included in the design for the collation
> facility.  That might've been a mistake ...
>

In this case the hammer seems very large, since only one side of the '<-'
operator is even collatable. Mind you, if it *did* work it would still
bubble up to a case of 'text = text' at the top node, so the problem would
still remain. Although it seems unfair: I can definite declare a table with
a text column and run a query with Const = Var and it'll ship that OpExpr
over, which seems no more fishy than what I'm asking the hstore to do.

hstore=# explain (verbose) select * from txt_fdw where txt = 'this';
QUERY PLAN
---
 Foreign Scan on public.txt_fdw  (cost=100.00..127.20 rows=7 width=36)
   Output: id, txt
   Remote SQL: SELECT id, txt FROM public.txt WHERE ((txt = 'this'::text))
(3 rows)

The fdw table can't know much about what the remote collation is, it only
knows what I've told it locally which is that it's the default, so
everything matches up. Sounds like the problem is hstore lacking a
collation? Or, that lacking a collation is not considered equivalent to the
default in the testing code.

P


P


>
> regards, tom lane
>

Re: [HACKERS] [PATCH] pgpassfile connection option

[Julian Markwort]

Fabien Coelho wrote:

A few detailed comments:

Beware of spacing:
 . "if(" -> "if (" (2 instances)
 . " =='\0')" -> " == '\0')" (at least one instance)

Elsewhere:

 + if (conn->pgpassfile_used && conn->password_needed && conn->result &&
 + conn->pgpassfile && conn->pgpassfile[0]!='\0' &&

ISTM that if pgpassfile_used is true then pgpassfile is necessarily 
defined, so the second line tests are redundant? Or am I missing 
something? 

I've adressed those spacing errors.
You are right, if pgpassfile_used is true, it SHOULD be defined, I just 
like to be careful whenever I'm working with strings. But I guess in 
this scenario I can trust the caller and omit those checks.



On 11/22/2016 07:04 AM, Mithun Cy wrote:
 > Independently of your patch, while testing I concluded that the 
multi-host feature and documentation should be improved:

 > - If a connection fails, it does not say for which host/port.

Thanks I will re-test same.

> In fact they are tried in turn if the network connection fails, but not
> if the connection fails for some other reason such as the auth.
> The documentation is not precise enough.

If we fail due to authentication, then I think we should notify the 
client instead of trying next host. I think it is expected genuine 
user have right credentials for making the connection. So it's better 
if we notify same to client immediately rather than silently ignoring 
them. Otherwise the host node which the client failed to connect will 
be permanently unavailable until client corrects itself. But I agree 
documentation and error message as you said need improvements. I will 
try to correct same
I agree with those criticisms of the multi-host feature and notifying 
the client in case of an authentification error rather than trying other 
hosts seems sensible to me.
But I think fixes for those should be part of different patches, as this 
patch's aim was only to expand the existing pgpassfile functionality to 
be used with a parameter.


regards,
Julian
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 0f375bf..c806aeb 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -943,7 +943,7 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
 Note that authentication is likely to fail if host
 is not the name of the server at network address hostaddr.
 Also, note that host rather than hostaddr
-is used to identify the connection in ~/.pgpass (see
+is used to identify the connection in a password file (see
 ).

 
@@ -1002,6 +1002,16 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
   
  
 
+ 
+  pgpassfile
+  
+  
+Specifies the name of the file used to lookup passwords.
+Defaults to the password file (see ).
+  
+  
+ 
+
  
   connect_timeout
   
@@ -6886,9 +6896,8 @@ myEventProc(PGEventId evtId, void *evtInfo, void *passThrough)
   
PGPASSFILE
   
-  PGPASSFILE specifies the name of the password file to
-  use for lookups.  If not set, it defaults to ~/.pgpass
-  (see ).
+  PGPASSFILE behaves the same as the  connection parameter.
  
 
 
@@ -7160,13 +7169,15 @@ myEventProc(PGEventId evtId, void *evtInfo, void *passThrough)
   
 
   
-   The file .pgpass in a user's home directory or the
-   file referenced by PGPASSFILE can contain passwords to
+   The file .pgpass in a user's home directory can contain passwords to
be used if the connection requires a password (and no password has been
specified  otherwise). On Microsoft Windows the file is named
%APPDATA%\postgresql\pgpass.conf (where
%APPDATA% refers to the Application Data subdirectory in
the user's profile).
+   Alternatively, a password file can be specified
+   using the connection parameter 
+   or the environment variable PGPASSFILE.
   
 
   
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 3e9c45b..ffc341d 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -184,6 +184,10 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
 		"Database-Password", "*", 20,
 	offsetof(struct pg_conn, pgpass)},
 
+	{"pgpassfile", "PGPASSFILE", NULL, NULL,
+		"Database-Password-File", "", 64,
+	offsetof(struct pg_conn, pgpassfile)},
+
 	{"connect_timeout", "PGCONNECT_TIMEOUT", NULL, NULL,
 		"Connect-timeout", "", 10,		/* strlen(INT32_MAX) == 10 */
 	offsetof(struct pg_conn, connect_timeout)},
@@ -374,10 +378,10 @@ static int parseServiceFile(const char *serviceFile,
  PQExpBuffer errorMessage,
  bool *group_found);
 static char *pwdfMatchesString(char *buf, char *token);
-static char *PasswordFromFile(char *hostname, char *port, char *dbname,
- char *username);
-static bool getPgPassFilename(char *pgpassfile);
-static void dot_pg_pass_warning(PGconn *conn);
+static char *passwordFromFile(char *hostname, char *port, char *dbname,
+ char 

Re: [HACKERS] Tackling JsonPath support

[Pavel Stehule]

2016-11-27 17:50 GMT+01:00 Christian Convey :

> From looking at other databases' docs, it seems like the behavior of
> various JSON-related operators / functions are described partially in terms
> of a "json path expression":
>
> * In Oracle, "JSON_TABLE", "JSON_exists_column", "JSON_value_column": [1]
> * In MySQL: [2]
> * In DB2: [3]
> * In MS SQL Server: [4]
> * (Whatever the Standards committee will end up producing.)
>
> If I'm correctly understanding the situation, It sounds like we have two
> big unknowns:
>
> (a) The exact syntax/semantics of JSON path searching, especially w.r.t.
> corner cases and error handling, and
>
> (b) The syntax/semantics of whatever SQL operators / functions are
> currently defined in terms of (a).  E.g., "JSON_TABLE".
>
> If that's correct, then what do you guys think about us taking the
> following incremental approach?
>
> Step 1: I'll dig into the implementations described above, to see what's
> similar and different between the JSON-path-expression syntax and semantics
> offered by each.  I then report my findings here, and we can hopefully
> reach a consensus about the syntax/semantics of PG's json-path-expression
> handling.
>
> Step 2: I submit a patch for adding a new function to "contrib", which
> implements the JSON-path-expression semantics chosen in Step 1.  The
> function will be named such that people won't confuse it with any
> (eventual) SQL-standard equivalent.
>
> Step 3: PG developers can, if they choose, start defining new JSON
> operator / functions, and/or port existing JSON-related functions, in terms
> of the function created in Step 2.
>
> I see the following pros / cons to this approach:
>
> Pro: It gives us a concrete start on this functionality, even though we're
> not sure what's happening with the SQL standard.
>
> Pro: The risk of painting ourselves into a corner is relatively low,
> because we're putting the functionality in "contrib", and avoid function
> names which conflict with likely upcoming standards.
>
> Pro: It might permit us to give PG users access to JSONPath -like
> functionality sooner than if we wait until we're clear on the ideal
> long-term interface.
>

Incremental work is great idea - I like this this style. Instead contrib,
you can use public repository on github. Minimally for first stage is
better to live outside core - you are not restricted by PostgreSQL
development process. When your code will be stabilized, then you can go to
commitfest. I believe so we need good JSON support. The XML support helps
to PostgreSQL lot of, JSON will be great too.


>
> Con: "JSON path expression" is a recurring them in the *grammars* of
> user-facing operators in [1], [2], [3], and [4].  But it doesn't
> necessarily follow that the function implemented in Step 2 will provide
> useful infrastructure for PG's eventual implementations of "JSON_TABLE",
> etc.
>

We can implement subset only - our XPath based on libxml2 does it too. The
good target is support of usual examples on the net.

Regards

Pavel

>
> - Christian
>
> [1] https://docs.oracle.com/database/121/SQLRF/functions092.htm#SQLRF56973
> [2] https://dev.mysql.com/doc/refman/5.7/en/json-path-syntax.html
> [3] http://www.ibm.com/support/knowledgecenter/ssw_
> ibm_i_72/db2/rbafzjsonpath.htm
> [4] https://msdn.microsoft.com/en-us/library/mt577087.aspx
>

Re: [HACKERS] make default TABLESPACE belong to target table.

[Amit Kapila]

On Sat, Nov 26, 2016 at 9:46 PM, Tom Lane  wrote:
> Amit Kapila  writes:
>> What will such a storage parameter (default_tablespace) mean at table
>> level and how it will different from existing default_tablespace?  I
>> think the usage asked by Amos is quite genuine, but not sure if
>> introducing default_tablespace as a storage level parameter is the
>> best way to address it.  Another way could be that we allow the user
>> to specify something like tablespace_name /> table> or something like that.
>
> That seems overcomplicated, and it will also pose quite some hazard
> for pg_dump for example.  It feels like "action at a distance", in
> that creation of an index will now depend on properties that aren't
> immediately obvious.
>
> I was thinking about introducing a new GUC, named something like
> default_index_tablespace, which would need to have at least these
> behaviors:
>
> 1. index tablespace is same as default_tablespace (the backwards
> compatible, and therefore the default, behavior).
>
> 2. index tablespace is same as table's tablespace.
>
> 3. default_index_tablespace names a specific tablespace.
>
> Point 3 isn't in the current request but I'm pretty sure I've heard
> it requested in the past, so that people can conveniently put all
> tables in tablespace X and all indexes in tablespace Y.
>
> If we just did points 1 and 2 then a bool GUC would suffice.  I'm
> not sure how to handle all three cases cleanly.  We could define
> default_index_tablespace as empty to get point 1 or a tablespace
> name to get point 3, but that leaves us having to use some magic
> string for point 2, which would be messy --- what if it conflicts
> with someone's choice of a tablespace name?
>

Yeah, I think coming with a clean way to handle all three might be
messy.  How about if just handle 2 and 3?  If the table is created
with default_tablespace, then automatically it will be created in
default_tablespace.  Do you think maintaining backward compatibility
is important in this case?


-- 
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] pg_dump / copy bugs with "big lines" ?

[Daniel Verite]

Alvaro Herrera wrote:

> I propose to rename allow_long to huge_ok.  "Huge" is the terminology
> used by palloc anyway.  I'd keep makeLongStringInfo() and
> initLongStringInfo() though as interface, because using Huge instead of
> Long there looks strange.  Not wedded to that, though (particularly as
> it's a bit inconsistent).

"Long" makes sense to me as qualifying a limit greater than
MaxAllocSize but lower (or equal) than MaxAllocHugeSize.

In memutils.h we have these definitions:

#define MaxAllocSize   ((Size) 0x3fff)   /* 1 gigabyte - 1 */
#define MaxAllocHugeSize   ((Size) -1 >> 1) /* SIZE_MAX / 2 */

And in enlargeStringInfo() the patch adds this:
/*
 * Maximum size for strings with allow_long=true.
 * It must not exceed INT_MAX, as the StringInfo routines
 * expect offsets into the buffer to fit into an int.
 */
const int max_alloc_long = 0x7fff;

On a 32-bit arch, we can expect max_alloc_long == MaxAllocHugeSize,
but on a 64-bit arch, it will be much smaller with MaxAllocHugeSize
at (2^63)-1.

IOW, the patch only doubles the maximum size of StringInfo
whereas we could say that it should multiply it by 2^32 to pretend to
the "huge" qualifier.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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: session server side variables

[Pavel Stehule]

Hi

2016-11-28 10:39 GMT+01:00 Artur Zakirov :

> On 28.11.2016 10:42, Pavel Stehule wrote:
>
>>
>> next update - setattr, getattr functions are working now
>>
>> notes, comments?
>>
>> Regards
>>
>> Pavel
>>
>>
> It is interesting!
>
> Do you have plans to support also table variables? For example, like this:
>
> create type composite_type_2 as (a int, b text);
> create variable var7 composite_type_2;
> select insertvar('var7','(10,Hello world\, Hello world\, Hello world)');
> select insertvar('var7','(1000,Hola, hola!)');
> select * from getvar('var7');
>   a   |   b
> --+---
>  10   | Hello world, Hello world, Hello world
>  1000 | Hola, hola!
>
> Or it is a bad idea? Or it is not related to this patch?
>

Minimally in first stage I have not plan to support tables. It opens lot of
questions - lot of code to implement - how to implement indexes,
statistics, MVCC?

But some workaround is not hard - you can store a array of composite types.

 postgres=# select setvar('a', array(select row(10,'ahoj')::test from
generate_series(1,10)));
┌──
│
a
╞══
│
{"(10,ahoj)","(10,ahoj)","(10,ahoj)","(10,ahoj)","(10,ahoj)","(10,ahoj)","(10,ahoj)","(10,ahoj)","(10,ahoj)","(10,aho
└──
(1 row)

Time: 0,992 ms
postgres=# select * from unnest(getvar('a'));
┌┬──┐
│ a  │  b   │
╞╪══╡
│ 10 │ ahoj │
│ 10 │ ahoj │
│ 10 │ ahoj │
│ 10 │ ahoj │
│ 10 │ ahoj │
│ 10 │ ahoj │
│ 10 │ ahoj │
│ 10 │ ahoj │
│ 10 │ ahoj │
│ 10 │ ahoj │
└┴──┘
(10 rows)

For fast append it needs another significant work (and can be done in next
step), but almost all work did Tom already.



> We have the extension (https://github.com/postgrespro/pg_variables). And
> it supports table like variables. It shows better performance against
> temporary tables.
>
> --
> Artur Zakirov
> Postgres Professional: http://www.postgrespro.com
> Russian Postgres Company
>

Re: [HACKERS] Mail thread references in commits

[Magnus Hagander]

On Mon, Nov 28, 2016 at 3:36 AM, Tom Lane  wrote:

> Magnus Hagander  writes:
> > Ok, we now have it. https://postgr.es/m/messageid will redirect to that
> > messageid in the main archives.
>
> I pushed a patch using this new convention:
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=
> dafa0848da11260e9510c699e7060171336cb550
>
> While the URL seems to work, I notice that gitweb doesn't show it as
> a clickable link in the above page.  Is that fixable?
>

That seems to be because your message-id looks like a git commit hash. It
does the same with your messageid's in previous commits, does it not?

I don't really read perl enough to take it apart. But
http://git.kernel.org/cgit/git/git.git/tree/gitweb/gitweb.perl is the code
(we're probably on an older version). I'm guessing it's coming out of
format_log_line (
http://git.kernel.org/cgit/git/git.git/tree/gitweb/gitweb.perl#n2035). (the
version we have only has the part that looks for the hash).

Doesn't seem to be configurable. We can of course turn that off completely,
but in that case it will no longer match any other git hash references in
commit messages, so that might be a net loss.

I wonder if it's worth forking gitweb to make it do explicitly what we want
for this -- that is recognize all the different kinds of things that would
be interesting here. But that fork should probably be done by somebody with
some more perl skills than me :)

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

Re: [HACKERS] Quorum commit for multiple synchronous replication.

[Masahiko Sawada]

On Sat, Nov 26, 2016 at 10:27 PM, Michael Paquier
 wrote:
> On Tue, Nov 15, 2016 at 7:08 PM, Masahiko Sawada  
> wrote:
>> Attached latest version patch incorporated review comments. After more
>> thought, I agree and changed the value of standby priority in quorum
>> method so that it's not set 1 forcibly. The all standby priorities are
>> 1 If s_s_names = 'ANY(*)'.
>> Please review this patch.
>
> Sorry for my late reply. Here is my final lookup.

Thank you for reviewing!

>  
> -num_sync (  class="parameter">standby_name [, ...] )
> +[ANY] num_sync (
> standby_name [, ...] )
> +FIRST num_sync (
> standby_name [, ...] )
>  standby_name [, ...
> This can just be replaced with [ ANY | FIRST ]. There is no need for
> braces as the keyword is not mandatory.
>
> +is the name of a standby server.
> +FIRST and ANY specify the method used by
> +the master to control the standby servres.
>  
> s/servres/servers/.
>
> if (priority == 0)
> values[7] = CStringGetTextDatum("async");
> else if (list_member_int(sync_standbys, i))
> -   values[7] = CStringGetTextDatum("sync");
> +   values[7] = SyncRepConfig->sync_method == SYNC_REP_PRIORITY ?
> +   CStringGetTextDatum("sync") : 
> CStringGetTextDatum("quorum");
> else
> values[7] = CStringGetTextDatum("potential");
> This can be simplified a bit as "quorum" is the state value for all
> standbys with a non-zero priority when the method is set to
> SYNC_REP_QUORUM:
> if (priority == 0)
> values[7] = CStringGetTextDatum("async");
> +   else if (SyncRepConfig->sync_method == SYNC_REP_QUORUM)
> +   values[7] = CStringGetTextDatum("quorum");
> else if (list_member_int(sync_standbys, i))
> values[7] = CStringGetTextDatum("sync");
> else
>
> SyncRepConfig data is made external to syncrep.c with this patch as
> walsender.c needs to look at the sync method in place, no complain
> about that after considering if there could be a more elegant way to
> do things without this change.

Agreed.

> While reviewing the patch, I have found a couple of incorrectly shaped
> sentences, both in the docs and some comments. Attached is a new
> version with this word-smithing. The patch is now switched as ready
> for committer.

Thanks. I found a typo in v7 patch, so attached latest v8 patch.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index d8d207e..4baff32 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3032,42 +3032,76 @@ include_dir 'conf.d'
 transactions waiting for commit will be allowed to proceed after
 these standby servers confirm receipt of their data.
 The synchronous standbys will be those whose names appear
-earlier in this list, and
+in this list, and
 that are both currently connected and streaming data in real-time
 (as shown by a state of streaming in the
 
-pg_stat_replication view).
-Other standby servers appearing later in this list represent potential
-synchronous standbys. If any of the current synchronous
-standbys disconnects for whatever reason,
-it will be replaced immediately with the next-highest-priority standby.
-Specifying more than one standby name can allow very high availability.
+pg_stat_replication view). If the keyword
+FIRST is specified, other standby servers appearing
+later in this list represent potential synchronous standbys.
+If any of the current synchronous standbys disconnects for
+whatever reason, it will be replaced immediately with the
+next-highest-priority standby. Specifying more than one standby
+name can allow very high availability.


 This parameter specifies a list of standby servers using
 either of the following syntaxes:
 
-num_sync ( standby_name [, ...] )
+[ ANY | FIRST ] num_sync ( standby_name [, ...] )
 standby_name [, ...]
 
 where num_sync is
 the number of synchronous standbys that transactions need to
 wait for replies from,
 and standby_name
-is the name of a standby server. For example, a setting of
-3 (s1, s2, s3, s4) makes transaction commits wait
-until their WAL records are received by three higher-priority standbys
-chosen from standby servers s1, s2,
-s3 and s4.
+is the name of a standby server.
+FIRST and ANY specify the method used by
+the master to control the standby servers.
 
 
-The second syntax was used before PostgreSQL
+The keyword FIRST, coupled with num_sync,
+  

Re: [HACKERS] Physical append-only tables

[Masahiko Sawada]

On Mon, Nov 28, 2016 at 9:05 AM, Jim Nasby  wrote:
> On 11/24/16 8:18 AM, Bruce Momjian wrote:
>>>
>>> What if we used BRIN to find heap pages where the new row was in the
>>> page BRIN min/max range, and the heap page had free space.  Only if
>>> that
>>> fails do we put is somewhere else in the heap.
>>>
>>>
>>> That would certainly be useful. You'd have to figure out what to do in
>>> the case
>>> of multiple conflicting BRIN indexes (which you shouldn't have in the
>>> first
>>> place, but that won't keep people from having them), but other than that
>>> it
>>> would be quite good I think.
>>
>> This idea is only possible because the BRIN index is so small and easy
>> to scan, i.e. this wouldn't work for a btree index.
>
>
> ISTM a prerequisite for any of this is a way to override the default FSM
> behavior. A simple strategy that forces append-only would presumably be very
> cheap and easy to do after that. It could also be used to allow better
> clustering. It would also make it far easier to recover from a heavily
> bloated table that's too large to simply VACUUM FULL or CLUSTER, without
> resorting to the contortions that pg_repack/pg_reorg have to.

Since building BRIN index doesn't take a long time, it would be good
enough to support the online-clustering (or clustering with minimal
lock like pg_repack/pg_reorg does) in most cases. And I'm not sure
that there are a lot of users who build only BRIN index on the table.
I think that many users want to build other indexes on same table
other columns.

Regards,

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


-- 
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] Push down more full joins in postgres_fdw

[Etsuro Fujita]

On 2016/11/24 21:45, Etsuro Fujita wrote:

On 2016/11/22 18:28, Ashutosh Bapat wrote:

The comments should explain why is the assertion true.
+/* Shouldn't be NIL */
+Assert(tlist != NIL);



I noticed that I was wrong; in the Assertion the tlist can be empty.  An
example for such a case is:

SELECT 1 FROM (SELECT c1 FROM ft4 WHERE c1 between 50 and 60) t1 FULL
JOIN (SELECT c1 FROM ft5 WHERE c1 between 50 and 60) t2 ON (TRUE);

In this example any columns of the relations ft4 and ft5 wouldn't be
needed for the join or the final output, so both the tlists for the
relations created in deparseRangeTblRef would be empty.  Attached is an
updated version fixing this bug.  Changes are:

* I removed make_outerrel_subquery/make_innerrel_subquery from fpinfo
and added a new member "is_subquery_rel" to fpinfo, as a flag on whether
to deparse the relation as a subquery.
* I modified deparseRangeTblRef, deparseSelectSql, and is_subquery_var
to see the is_subquery_rel, not
make_outerrel_subquery/make_innerrel_subquery.
* I modified appendSubselectAlias to handle the case where ncols = 0
properly.
* I renamed subquery_rels in fpinfo to lower_subquery_rels, to make it
easy to distinguish this from is_subquery_rel clearly.
* I added regression tests for that.


I noticed that the above changes are not adequate; the updated patch 
breaks EXPLAIN outputs for EvalPlanQual subplans of foreign joins in 
which foreign tables are full joined and in the remote join query the 
foreign tables are deparsed as subqueries.  Consider:


postgres=# explain verbose select * from test, ((select * from ft1 where 
b between 1 and 3) t1 full join (select * from ft2 where b between 1 and 
3) t2

 on (t1.a = t2.a)) ss for update of test;


QUERY PLA
N
---

 LockRows  (cost=100.00..287.33 rows=6780 width=94)
   Output: test.a, test.b, ft1.a, ft1.b, ft2.a, ft2.b, test.ctid, 
ft1.*, ft2.*

   ->  Nested Loop  (cost=100.00..219.53 rows=6780 width=94)
 Output: test.a, test.b, ft1.a, ft1.b, ft2.a, ft2.b, test.ctid, 
ft1.*, ft2.*

 ->  Seq Scan on public.test  (cost=0.00..32.60 rows=2260 width=14)
   Output: test.a, test.b, test.ctid
 ->  Materialize  (cost=100.00..102.19 rows=3 width=80)
   Output: ft1.a, ft1.b, ft1.*, ft2.a, ft2.b, ft2.*
   ->  Foreign Scan  (cost=100.00..102.17 rows=3 width=80)
 Output: ft1.a, ft1.b, ft1.*, ft2.a, ft2.b, ft2.*
 Relations: (public.ft1) FULL JOIN (public.ft2)
 Remote SQL: SELECT s5.c1, s5.c2, s5.c3, s6.c1, 
s6.c2, s6.c3 FROM ((SELECT a, b, ROW(a, b) FROM public.t1 WHERE ((b >= 
1)) AND ((b
<= 3))) s5(c1, c2, c3) FULL JOIN (SELECT a, b, ROW(a, b) FROM public.t2 
WHERE ((b >= 1)) AND ((b <= 3))) s6(c1, c2, c3) ON (((s5.c1 = s6.c1
 ->  Hash Full Join  (cost=201.14..202.29 rows=3 
width=80)

   Output: ft1.a, ft1.b, ft1.*, ft2.a, ft2.b, ft2.*
   Hash Cond: (ft1.a = ft2.a)
   ->  Foreign Scan on public.ft1 
(cost=100.00..101.11 rows=3 width=40)

 Output: ft1.a, ft1.b, ft1.*
 Remote SQL: SELECT NULL FROM public.t1 
WHERE ((b >= 1)) AND ((b <= 3))

   ->  Hash  (cost=101.11..101.11 rows=3 width=40)
 Output: ft2.a, ft2.b, ft2.*
 ->  Foreign Scan on public.ft2 
(cost=100.00..101.11 rows=3 width=40)

   Output: ft2.a, ft2.b, ft2.*
   Remote SQL: SELECT NULL FROM 
public.t2 WHERE ((b >= 1)) AND ((b <= 3))

(23 rows)

The SELECT clauses of the remote queries for the Foreign Scans in the 
EPQ subplan are deparsed incorrectly into "SELECT NULL".  The reason for 
that is that since the foreign tables' fpinfo->is_subquery_rel has been 
set true at the time those clauses are deparsed (due to that the foreign 
tables have been deparsed as subqueries in the main remote join query), 
deparseSelectSql calls deparseExplicitTargetList with an empty tlist, to 
construct the clauses, so it deparses the tlist into "NULL".  This is 
harmless because we don't use the remote queries for the Foreign Scans 
during an EPQ recheck, but that would be confusing for users, so I 
modified the patch a bit further to make the EXPLAIN output as-is. 
Attached is a new version of the patch.



I also revised code and comments a bit, just for consistency.


Also, I renamed appendSubselectAlias to appendSubqueryAlias, because we 
use the term "subquery" for other places.



I will update another patch for PHVs on top of the 

Re: [HACKERS] Radix tree for character conversion

[Kyotaro HORIGUCHI]

Hello.

I'll be off line until at least next Monday. So I move this to
the next CF by myself.

At Wed, 09 Nov 2016 17:38:53 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20161109.173853.77274443.horiguchi.kyot...@lab.ntt.co.jp>
> Hello, thank you for polishing this.
> 
> At Wed, 9 Nov 2016 02:19:01 +0100, Daniel Gustafsson  wrote 
> in <80f34f25-bf6d-4bcd-9c38-42ed10d3f...@yesql.se>
> > > On 08 Nov 2016, at 17:37, Peter Eisentraut 
> > >  wrote:
> > > 
> > > On 10/31/16 12:11 PM, Daniel Gustafsson wrote:
> > >> I took a small stab at doing some cleaning of the Perl scripts, mainly 
> > >> around
> > >> using the more modern (well, modern as in +15 years old) form for 
> > >> open(..),
> > >> avoiding global filehandles for passing scalar references and enforcing 
> > >> use
> > >> strict.  Some smaller typos and fixes were also included.  It seems my 
> > >> Perl has
> > >> become a bit rusty so I hope the changes make sense.  The produced files 
> > >> are
> > >> identical with these patches applied, they are merely doing cleaning as 
> > >> opposed
> > >> to bugfixing.
> > >> 
> > >> The attached patches are against the 0001-0006 patches from Heikki and 
> > >> you in
> > >> this series of emails, the separation is intended to make them easier to 
> > >> read.
> > > 
> > > Cool.  See also here:
> > > https://www.postgresql.org/message-id/55E52225.4040305%40gmx.net
> 
> > Nice, not having hacked much Perl in quite a while I had all but forgotten
> > about perlcritic.
> 
> I tried it on CentOS7. Installation failed saying that
> Module::Build is too old. It is yum-inatlled so removed it and
> installed it with CPAN. Again failed with many 'Could not create
> MYMETA files'. Then tried to install CPAN::Meta and it failed
> saying that CPAN::Meta::YAML is too *new*. That sucks.
> 
> So your patch is greately helpfull. Thank you.
> 
> | -my @mapnames = map { s/\.map//; $_ } values %plainmaps;
> | +my @mapnames = map { my $m = $_; $m =~ s/\.map//; $m } values %plainmaps;
> 
> It surprised me to know that perlcritic does such things.
> 
> > Running it on the current version of the patchset yields mostly warnings on
> > string values used in the require “convutils.pm” statement.  There were 
> > however
> > two more interesting reports: one more open() call not using the three
> > parameter form and an instance of map which alters the input value. 
> 
> Sorry for overlooking it.
> 
> > The latter
> > is not causing an issue since we don’t use the input list past the map but
> > fixing it seems like good form.
> 
> Agreed.
> 
> > Attached is a patch that addresses the perlcritic reports (running without 
> > any
> > special options).
> 
> Thanks. The attached patch contains the patch by perlcritic.
> 
> 0001,2,3 are Heikki's patch that are not modified since it is
> first proposed. It's a bit too big so I don't attach them to this
> mail (again).
> 
> https://www.postgresql.org/message-id/08e7892a-d55c-eefe-76e6-7910bc8dd...@iki.fi
> 
> 0004 is radix-tree stuff, applies on top of the three patches
> above.
> 
> There's a hidden fifth patch which of 20MB in size. But it is
> generated by running make in the Unicode directory.
> 
> [$(TOP)]$ ./configure ...
> [$(TOP)]$ make
> [Unicode]$ make
> [Unicode]$ make distclean
> [Unicode]$ git add .
> [Unicode]$ commit 
> === COMMITE MESSSAGE
> Replace map files with radix tree files.
> 
> These encodings no longer uses the former map files and uses new radix
> tree files. All existing authority files in this directory are removed.
> ===

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

-- 
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] asynchronous execution

[Kyotaro HORIGUCHI]

Hello,

I cannot respond until next Monday, so I move this to the next CF
by myself.

At Tue, 15 Nov 2016 20:25:13 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20161115.202513.268072050.horiguchi.kyot...@lab.ntt.co.jp>
> Hello, this is a maintenance post of reased patches.
> I added a change of ResourceOwnerData missed in 0005.
> 
> At Mon, 31 Oct 2016 10:39:12 +0900 (JST), Kyotaro HORIGUCHI 
>  wrote in 
> <20161031.103912.217430542.horiguchi.kyot...@lab.ntt.co.jp>
> > This a PoC patch of asynchronous execution feature, based on a
> > executor infrastructure Robert proposed. These patches are
> > rebased on the current master.
> > 
> > 0001-robert-s-2nd-framework.patch
> > 
> >  Roberts executor async infrastructure. Async-driver nodes
> >  register its async-capable children and sync and data transfer
> >  are done out of band of ordinary ExecProcNode channel. So async
> >  execution no longer disturbs async-unaware node and slows them
> >  down.
> > 
> > 0002-Fix-some-bugs.patch
> > 
> >  Some fixes for 0001 to work. This is just to preserve the shape
> >  of 0001 patch.
> > 
> > 0003-Modify-async-execution-infrastructure.patch
> > 
> >  The original infrastructure doesn't work when multiple foreign
> >  tables is on the same connection. This makes it work.
> > 
> > 0004-Make-postgres_fdw-async-capable.patch
> > 
> >  Makes postgres_fdw to work asynchronously.
> > 
> > 0005-Use-resource-owner-to-prevent-wait-event-set-from-le.patch
> > 
> >  This addresses a problem pointed by Robers about 0001 patch,
> >  that WaitEventSet used for async execution can leak by errors.
> > 
> > 0006-Apply-unlikely-to-suggest-synchronous-route-of-ExecA.patch
> > 
> >  ExecAppend gets a bit slower by penalties of misprediction of
> >  branches. This fixes it by using unlikely() macro.
> > 
> > 0007-Add-instrumentation-to-async-execution.patch
> > 
> >  As the description above for 0001, async infrastructure conveys
> >  tuples outside ExecProcNode channel so EXPLAIN ANALYZE requires
> >  special treat to show sane results. This patch tries that.
> > 
> > 
> > A result of a performance measurement is in this message.
> > 
> > https://www.postgresql.org/message-id/20161025.182150.230901487.horiguchi.kyot...@lab.ntt.co.jp
> > 
> > 
> > | t0  - SELECT sum(a) FROM ;
> > | pl  - SELECT sum(a) FROM <4 local children>;
> > | pf0 - SELECT sum(a) FROM <4 foreign children on single connection>;
> > | pf1 - SELECT sum(a) FROM <4 foreign children on dedicate connections>;
> > ...
> > | async
> > |   t0: 3885.84 ( 40.20)  0.86% faster (should be error but stable on my 
> > env..)
> > |   pl: 1617.20 (  3.51)  1.26% faster (ditto)
> > |  pf0: 6680.95 (478.72)  19.5% faster
> > |  pf1: 1886.87 ( 36.25)  77.1% faster

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

[HACKERS] Fix comment in build_simple_rel

[Amit Langote]

Attached fixes reference in a comment to a non-existent function:

s/GetRelationInfo/get_relation_info/g

Thanks,
Amit
diff --git a/src/backend/optimizer/util/relnode.c b/src/backend/optimizer/util/relnode.c
index deef560..d5326e6 100644
--- a/src/backend/optimizer/util/relnode.c
+++ b/src/backend/optimizer/util/relnode.c
@@ -126,7 +126,7 @@ build_simple_rel(PlannerInfo *root, int relid, RelOptKind reloptkind)
 	rel->allvisfrac = 0;
 	rel->subroot = NULL;
 	rel->subplan_params = NIL;
-	rel->rel_parallel_workers = -1;		/* set up in GetRelationInfo */
+	rel->rel_parallel_workers = -1;		/* set up in get_relation_info */
 	rel->serverid = InvalidOid;
 	rel->userid = rte->checkAsUser;
 	rel->useridiscurrent = false;

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

[HACKERS] A bug of psql completion

[Kyotaro HORIGUCHI]

Hello.

I noticed that the psql completion code for "ALTER TABLE x ALTER
[COLUMN] x DROP" is wrong. It works as the following

=# alter table x alter x drop 
[nothing suggested]
=# alter table x table x alter x drop 
DEFAULT   NOT NULL  

The attached patch fixes it.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index b556c00..6aa3f20 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1804,7 +1804,7 @@ psql_completion(const char *text, int start, int end)
 		COMPLETE_WITH_LIST4("PLAIN", "EXTERNAL", "EXTENDED", "MAIN");
 	/* ALTER TABLE ALTER [COLUMN]  DROP */
 	else if (Matches7("ALTER", "TABLE", MatchAny, "ALTER", "COLUMN", MatchAny, "DROP") ||
-			 Matches8("ALTER", "TABLE", MatchAny, "TABLE", MatchAny, "ALTER", MatchAny, "DROP"))
+			 Matches6("ALTER", "TABLE", MatchAny, "ALTER", MatchAny, "DROP"))
 		COMPLETE_WITH_LIST2("DEFAULT", "NOT NULL");
 	else if (Matches4("ALTER", "TABLE", MatchAny, "CLUSTER"))
 		COMPLETE_WITH_CONST("ON");

-- 
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: session server side variables

[Artur Zakirov]

On 28.11.2016 10:42, Pavel Stehule wrote:


next update - setattr, getattr functions are working now

notes, comments?

Regards

Pavel



It is interesting!

Do you have plans to support also table variables? For example, like this:

create type composite_type_2 as (a int, b text);
create variable var7 composite_type_2;
select insertvar('var7','(10,Hello world\, Hello world\, Hello world)');
select insertvar('var7','(1000,Hola, hola!)');
select * from getvar('var7');
  a   |   b
--+---
 10   | Hello world, Hello world, Hello world
 1000 | Hola, hola!

Or it is a bad idea? Or it is not related to this patch?

We have the extension (https://github.com/postgrespro/pg_variables). And 
it supports table like variables. It shows better performance against 
temporary tables.


--
Artur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company


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

Re: [HACKERS] Proposal: scan key push down to heap [WIP]

[Dilip Kumar]

On Mon, Nov 28, 2016 at 3:00 PM, Dilip Kumar  wrote:
> As promised, I have taken the performance with TPCH benchmark and
> still result are quite good. However this are less compared to older
> version (which was exposing expr ctx and slot to heap).
>
> Query   Head [1] Patch3   Improvement
> Q3   36122.425  32285.608 10%
> Q4   6797   5763.871   15%
> Q1017996.104  15878.505  11%
> Q1212399.6519969.489  19%
>
>  [1] heap_scankey_pushdown_POC_V3.patch : attached with the mail.

I forgot to mention the configuration parameter in last mail..

TPCH-scale factor 10.
work mem 20MB
Power, 4 socket machine
Shared Buffer 1GB


-- 
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