Re: [HACKERS] Transactions involving multiple postgres foreign servers
On Mon, Jan 30, 2017 at 12:50 PM, Ashutosh Bapat wrote: > On Sat, Jan 28, 2017 at 8:41 PM, Peter Eisentraut > wrote: >> On 1/26/17 4:49 AM, Masahiko Sawada wrote: >>> Sorry, I attached wrong version patch of pg_fdw_xact_resovler. Please >>> use attached patch. >> >> So in some other thread we are talking about renaming "xlog", because >> nobody knows what the "x" means. In the spirit of that, let's find >> better names for new functions as well. > > It's common in English (not just the database jargon) to abbreviate > "trans" by "x" [1]. xlog went a bit far by abbreviating whole > "transaction" by "x". But here "xact" means "transact", which is fine. > May be we should use 'X' instead of 'x', I don't know. Said that, I am > fine with any other name which conveys what the function does. > > [1] https://en.wikipedia.org/wiki/X > "txn" can be used for abbreviation of "Transaction", so for example pg_fdw_txn_resolver? I'm also fine to change the module and function name. 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] Review: GIN non-intrusive vacuum of posting tree
2017-01-25 12:09 GMT+05:00 Andrew Borodin : > 2017-01-24 22:29 GMT+05:00 Jeff Davis : >> By the way, can you show me where the Lehman and Yao paper addresses >> page recycling? > > Here J. Hellerstein comments L&Y paper [1] saying that effectively > there is no deletion algorithm provided. > Here [2] is paper referenced from nbtree docs. I'll check if this is > applicable to GIN B-tree. > > [1] http://db.cs.berkeley.edu/jmh/cs262b/treeCCR.html > [2] http://dl.acm.org/citation.cfm?id=324589 Hi! I'll summarize here my state of studying concurrent methods of page unlinking. GIN B-tree does not have "high key". That means, that rightmost key on a page is maximal for the non-leaf page. But I do not see anything theoretical in a way of implementation of Lanin and Shasha`s methods of page merging, with slight modifications. Their paper does not even mention high key(high fence key in papers by Goetz Graefe). But it's not a simple task due to large portions of shared code between entry tree and posting tree. Also, I do not see a reason why this method can be practically superior to proposed patch. Currently, I do not have resources to implement a proof of concept for fully concurrent page unlinking to make benchmarking. Best regards, Andrey Borodin. -- 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_stat_statements query normalization issues with combined queries
Mmm.. At Sat, 28 Jan 2017 11:52:20 +0800, Craig Ringer wrote in > On 28 Jan. 2017 02:08, "Tom Lane" wrote: > > Kyotaro HORIGUCHI writes: > > By the way the existing comment for the hook, > > >> * > >> * We provide a function hook variable that lets loadable plugins get > >> * control when ProcessUtility is called. Such a plugin would normally > >> * call standard_ProcessUtility(). > >> */ > > > This is quite a matter of course for developers. planner_hook and > > other ones don't have such a comment or have a one-liner at > > most. Since this hook gets a good amount of commnet, it seems > > better to just remove the existing one.. > > Hm? I see pretty much the same wording for planner_hook: Sorry, my eyes were very short-ranged. Surely most of them have commnents with very similar phrase. ExplainOneQuery seems to be one exception but I'll call ExplainOneQuery in the hook function, though:p Anyway I don't want to remove the comment in ProcessUtility since now I know that is rather the common case. > * To support loadable plugins that monitor or modify planner behavior, > * we provide a hook variable that lets a plugin get control before and > * after the standard planning process. The plugin would normally call > * standard_planner(). > > I think other places have copied-and-pasted this as well. > > The real problem with this is it isn't a correct specification of the > contract: in most cases, the right thing to do is more like "call the > previous occupant of the ProcessUtility_hook, or standard_ProcessUtility > if there was none." > > Not sure if it's worth trying to be more precise in these comments, > but if we do something about it, we need to hit all the places with > this wording. That seems bad. I'll prefer rather leaving them alone. Modifying them wouldn't be so much gain if many of them already have such comment. > I'd rather leave it for the hooks documentation work that's being done > elsewhere and have a README.hooks or something that discusses patterns > common across hooks. Then we can just reference it. > > Otherwise we'll have a lot of repetitive comments. Especially once we > mention that you can also ERROR out or (potentially) take no action at all. Sorry for the noise.. regards, -- 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] Create a separate test file for exercising system views
On Mon, Jan 30, 2017 at 2:32 AM, Tom Lane wrote: > In connection with the "pg_hba_file_settings view patch" thread, I was > wondering where we could logically insert a regression test case for that > view. I realized that there is no natural home for it among the existing > regression tests, because it's not really connected to any SQL language > feature. The same is true for a number of other built-in views, and > unsurprisingly, most of them are not exercised anywhere :-(. > > Accordingly, I propose creating a new regression test file whose charter > is to exercise the SRFs underlying system views, as per attached. > I don't think we desperately need new tests for views that expand to > simple SQL, but these test cases correspond directly to code coverage > for C functions, so they seem worthwhile. > > I did not do anything about testing the various pg_stat_xxx views. > Those could be added later, or maybe they deserve their own home. > (In many cases, those would need something smarter than the basic > count(*) technique used here, because the C functions are invoked > in the view's SELECT list not in FROM, so the planner would throw > away those calls.) > > Comments/objections? > I think this is good in the given infrastructure. Thanks for working on it. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database 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] Radix tree for character conversion
Hello, this is the revised version of character conversion using radix tree.
At Fri, 27 Jan 2017 17:33:57 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI
wrote in
<20170127.173357.221584433.horiguchi.kyot...@lab.ntt.co.jp>
> Hi, this is an intermediate report without a patch.
>
> At Thu, 26 Jan 2017 21:42:12 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI
> wrote in
> <20170126.214212.111556326.horiguchi.kyot...@lab.ntt.co.jp>
> > > > 0003-Apply-pgperltidy-on-src-backend-utils-mb-Unicode.patch
> > > >
> > > > Before adding radix tree stuff, applied pgperltidy and inserted
> > > > format-skipping pragma for the parts where perltidy seems to do
> > > > too much.
> > >
> > > Which version of perltidy did you use? Looking at the archives, the
> > > perl code is cleaned up with a specific version, v20090616. See
> > > https://www.postgresql.org/message-id/20151204054322.ga2070...@tornado.leadboat.com
> > > for example on the matter. As perltidy changes over time, this may be
> > > a sensitive change if done this way.
>
> My perltidy -v said "v20121207'. Anyway, I gave up to apply
> perltidy by myself. So I'll just drop 0003 and new 0004 (name
> changed to 0003) is made immediately on 0002.
I'm not sure what to handle this so I just removed the perltidy
stuff from this patchset.
> > > > 0004-Use-radix-tree-for-character-conversion.patch
> > > >
> > > > Radix tree body.
> > >
> > > Well, here a lot of diffs could have been saved.
> > >
> > > > The unattached fifth patch is generated by the following steps.
> > > >
> > > > [$(TOP)]$ ./configure
> > > > [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.
> > > > ===
> > >
> > > OK, I can see that working, with 200k of maps generated.. So going
> > > through the important bits of this jungle..
> >
> > Many thaks for the exploration.
> >
> > > +/*
> > > + * radix tree conversion function - this should be identical to the
> > > function in
> > > + * ../conv.c with the same name
> ..
> > > This is not nice. Having a duplication like that is a recipe to forget
> > > about it as this patch introduces a dependency with conv.c and the
> > > radix tree generation.
>
> In the attatched patch, mb/char_conveter.c which contains one
> inline function is created and it is includ'ed from mb/conv.c and
> mb/Unicode/map_checker.c.
>
> > > Having a .gitignore in Unicode/ would be nice, particularly to avoid
> > > committing map_checker.
>
> I missed this. I added .gitignore to ignore map_checker stuff
> and authority files and old-style map files.
>
> > > A README documenting things may be welcome, or at least comments at
> > > the top of map_checker.c. Why is map_checker essential? What does it
> > > do? There is no way to understand that easily, except that it includes
> > > a "radix tree conversion function", and that it performs sanity checks
> > > on the radix trees to be sure that they are on a good shape. But as
> > > this something that one would guess only after looking at your patch
> > > and the code (at least I will sleep less stupid tonight after reading
> > > this stuff).
> >
> > Okay, I'll do that.
>
> The patch has not been provided yet, I'm going to put the
> following comment just before the main() in map_checker.c.
>
> /*
> * The old-style plain map files were error-resistant due to its
> * straight-forward way for generation from authority files. In contrast the
> * radix tree maps are generated by a rather complex calculation and have a
> * complex, hard-to-confirm format.
> *
> * This program runs sanity check of the radix tree maps by confirming all
> * characters in the plain map files to be converted to the same code by the
> * corresponding radix tree map.
> *
> * All map files are included by map_checker.h that is generated by the script
> * make_mapchecker.pl as the variable mappairs.
> *
> */
>
>
> I'll do the following things later.
The following is the continuation.
> > --- a/src/backend/utils/mb/Unicode/UCS_to_SJIS.pl
> > +++ b/src/backend/utils/mb/Unicode/UCS_to_SJIS.pl
> > # Drop these SJIS codes from the source for UTF8=>SJIS conversion
> > #<<< do not let perltidy touch this
> > -my @reject_sjis =(
> > +my @reject_sjis = (
> > 0xed40..0xeefc, 0x8754..0x875d, 0x878a, 0x8782,
> > - 0x8784, 0xfa5b, 0xfa54, 0x8790..0x8792, 0x8795..0x8797,
> > + 0x8784, 0xfa5b, 0xfa54, 0x8790..0x8792, 0x8795..0x8797,
> > 0x879a..0x879c
> > -);
> > + );
> > This is not generated, it would be nice to drop the noise from the patch.
>
> Mmm. I'm not sure how this is generated but I'll care for that.
I don't still understand what what the intermediate diff comes
from but copy-n-pasting from master silenced it...
> > Here is another one:
> > - $i->{code} = $jis | (
> >
Re: [HACKERS] Query fails when SRFs are part of FROM clause (Commit id: 69f4b9c85f)
On Sat, Jan 28, 2017 at 3:43 AM, David G. Johnston <
david.g.johns...@gmail.com> wrote:
> On Fri, Jan 27, 2017 at 5:28 AM, Rushabh Lathia
> wrote:
>
>> Consider the below test;
>>
>> CREATE TABLE tab ( a int primary key);
>>
>> SELECT *
>> FROM pg_constraint pc,
>> CAST(CASE WHEN pc.contype IN ('f','u','p') THEN generate_series(1,
>> array_upper(pc.conkey, 1)) ELSE NULL END AS int) AS position;
>>
>> Above query is failing with "set-valued function called in context that
>> cannot
>> accept a set". But if I remove the CASE from the query then it working
>> just good.
>>
>> Like:
>>
>> SELECT *
>> FROM pg_constraint pc,
>> CAST(generate_series(1, array_upper(pc.conkey, 1)) AS int) AS position;
>>
>> This started failing with 69f4b9c85f168ae006929eec44fc44d569e846b9. It
>> seems
>> check_srf_call_placement() sets the hasTargetSRFs flag and but when the
>> SRFs
>> at the rtable ofcourse this flag doesn't get set. It seems like missing
>> something
>> their, but I might be completely wrong as not quire aware of this area.
>>
>>
> I'm a bit surprised that your query actually works...and without delving
> into source code its hard to explain why it should/shouldn't or whether the
> recent SRF work was intended to impact it.
>
> In any case the more idiomatic way of writing your query these days (since
> 9.4 came out) is:
>
> SELECT *
> FROM pg_constraint pc
> LEFT JOIN LATERAL generate_series(1, case when contype in ('f','p','u')
> then array_upper(pc.conkey, 1) else 0 end) gs ON true;
>
> generate_series is smart enough to return an empty set (instead of
> erroring out) when provided with (1,0) as arguments.
>
>
Thanks for the providing work-around query and I also understood your point.
At the same time reason to raise this issue was, because this was working
before
69f4b9c85f168ae006929eec44fc44d569e846b9 commit and now its throwing
an error. So whether its intended or query started failing because of some
bug introduced with the commit.
Issues is reproducible when query re-written with LEFT JOIN LATERAL and I
continue to use CASE statement.
SELECT *
FROM pg_constraint pc
LEFT JOIN LATERAL CAST((CASE WHEN pc.contype IN ('f','u','p') THEN
generate_series(1, array_upper(pc.conkey, 1)) ELSE NULL END) AS int) gs ON
true;
ERROR: set-valued function called in context that cannot accept a set
David J.
>
>
--
Rushabh Lathia
www.EnterpriseDB.com
Re: [HACKERS] WIP: About CMake v2
On 2017-01-27 22:20:41 -0500, Peter Eisentraut wrote: > On 1/27/17 6:11 PM, Andres Freund wrote: > > On 2017-01-27 09:09:36 -0500, Peter Eisentraut wrote: > >> My preferred scenario would be to replace the Windows build system by > >> this first, then refine it, then get rid of Autoconf. > >> > >> The ideal timeline would be to have a ready patch to commit early in a > >> development cycle, then get rid of the Windows build system by the end > >> of it. Naturally, this would need buy-in from Windows developers. > >> > >> I don't foresee replacing the Autoconf build system by this immediately. > > > > I'm very strongly against this path, it seems way too likely that we'll > > end up with yet another fragile thing that nobody from the *nix side > > will be able to test. > > That's a fair concern, but at least with CMake, someone from the *nix > side *can* test it, whereas right now it's completely separate. Given that fact, I just don't buy why it's a good idea to not also replace autoconf initially. Either we're able to properly test it - i.e. it runs all tests - on *nix or we're not. There's not a a whole of effort between those if you also want to do the windows side of things properly. > What kind of strategy do you have in mind? Do all of it. I'm unconvinced that a windows only version buys us meaningful savings, and I think the dangers of adding more duplication (msvc stuff after all gets some information from the makefiles) and long-term coexistence are quite severe. Greetings, Andres Freund -- 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] WIP: About CMake v2
On Sat, Jan 28, 2017 at 12:20 PM, Peter Eisentraut wrote: > On 1/27/17 6:11 PM, Andres Freund wrote: >> On 2017-01-27 09:09:36 -0500, Peter Eisentraut wrote: >>> My preferred scenario would be to replace the Windows build system by >>> this first, then refine it, then get rid of Autoconf. >>> >>> The ideal timeline would be to have a ready patch to commit early in a >>> development cycle, then get rid of the Windows build system by the end >>> of it. Naturally, this would need buy-in from Windows developers. >>> >>> I don't foresee replacing the Autoconf build system by this immediately. >> >> I'm very strongly against this path, it seems way too likely that we'll >> end up with yet another fragile thing that nobody from the *nix side >> will be able to test. > > That's a fair concern, but at least with CMake, someone from the *nix > side *can* test it, whereas right now it's completely separate. And people complain all the time that the MSVC build scripts are hacky and complicated.. So by beginning from there we switch from one build to the other, not increasing the number of builds that need to be maintained. Based on that Peter's strategy looks appealing to me. By the way, I am marking the patch as returned with feedback for this 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] pg_hba_file_settings view patch
On Mon, Jan 30, 2017 at 11:20 AM, Haribabu Kommi
wrote:
> On Sun, Jan 29, 2017 at 9:18 AM, Tom Lane wrote:
>> tgl wrote:
>> > I spent awhile hacking on this, and made a lot of things better, but
>> > I'm still very unhappy about the state of the comments.
>>
>> I made another pass over this, working on the comments and the docs,
>> and changing the view name to "pg_hba_file_rules". I think this version
>> is committable if people are satisfied with that name.
(catching up with this thread as a lot has happened.)
> Thanks for working on the patch. I am fine with the "pg_hba_file_rules"
> name. I have to improve in writing better comments after checking the
> attached patch. I will improve the comments in further patch submissions
> to community.
No objections here.
+/*
+ * The following character array represents the names of the authentication
+ * methods that are supported by PostgreSQL.
+ *
+ * Note: keep this in sync with the UserAuth enum in hba.h.
+ */
+static const char *const UserAuthName[] =
+{
+ "reject",
+ "implicit reject", /* Not a user-visible option */
+ "trust",
+ "ident",
+ "password",
+ "md5",
+ "gss",
+ "sspi",
+ "pam",
+ "bsd",
+ "ldap",
+ "cert",
+ "radius",
+ "peer"
+};
Perhaps this could use a StaticAssertStmt()? Say something like that:
#define USER_AUTH_LAST uaPeer
StaticAssertStmt(lengthof(UserAuthName) == USER_AUTH_LAST + 1,
"UserAuthName must include all user authentication names");
Any updates could easily be forgotten.
--
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] sequence data type
On Sat, Jan 28, 2017 at 2:49 AM, Peter Eisentraut
wrote:
> On 1/25/17 11:57 PM, Michael Paquier wrote:
>> @@ -15984,6 +15992,9 @@ dumpSequence(Archive *fout, TableInfo *tbinfo)
>> "CREATE SEQUENCE %s\n",
>> fmtId(tbinfo->dobj.name));
>>
>> + if (strcmp(seqtype, "bigint") != 0)
>> + appendPQExpBuffer(query, "AS %s\n", seqtype);
>> Wouldn't it be better to assign that unconditionally? There is no
>> reason that a dump taken from pg_dump version X will work on X - 1 (as
>> there is no reason to not make the life of users uselessly difficult
>> as that's your point), but that seems better to me than rely on the
>> sequence type hardcoded in all the pre-10 dump queries for sequences.
>
> Generally, we don't add default values, to keep the dumps from being too
> verbose.
>
> (I also think that being able to restore dumps to older versions would
> be nice, but that's another discussion.)
Okay. Fine for me.
>> Could you increase the regression test coverage to cover some of the
>> new code paths? For example such cases are not tested:
>> =# create sequence toto as smallint;
>> CREATE SEQUENCE
>> =# alter sequence toto as smallint maxvalue 1000;
>> ERROR: 22023: RESTART value (2147483646) cannot be greater than MAXVALUE
>> (1000)
>> LOCATION: init_params, sequence.c:1537
>
> Yeah, I had taken some notes along the way to add more test coverage, so
> since you're interested, attached is a patch. It's independent of the
> current patch and overlaps slightly. The example you show isn't really
> a new code path, just triggered differently, but the enhanced tests will
> cover it nonetheless.
Sure. Thanks for looking into that and getting a patch out. Oh, I have
just noticed that sequence_1.out has been removed by 9c18104c. That's
nice.
>> =# alter sequence toto as smallint;
>> ERROR: 22023: MAXVALUE (2147483647) is too large for sequence data
>> type smallint
>> LOCATION: init_params, sequence.c:1407
>>
>> + if ((seqform->seqtypid == INT2OID && seqform->seqmin < PG_INT16_MIN)
>> + || (seqform->seqtypid == INT4OID && seqform->seqmin < PG_INT32_MIN)
>> + || (seqform->seqtypid == INT8OID && seqform->seqmin < PG_INT64_MIN))
>> + {
>> + charbufm[100];
>> +
>> + snprintf(bufm, sizeof(bufm), INT64_FORMAT, seqform->seqmin);
>> +
>> + ereport(ERROR,
>> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>> +errmsg("MINVALUE (%s) is too large for sequence data type
>> %s",
>> + bufm, format_type_be(seqform->seqtypid;
>> + }
>> "large" does not apply to values lower than the minimum, no? The int64
>> path is never going to be reached (same for the max value), it doesn't
>> hurt to code it I agree.
>
> Yeah, I think that should be rephrased as something like "out of
> bounds", which is the term used elsewhere.
OK, that sounds good.
Looking at the patch adding some new tests, the coverage really
increases (I did not run make coverage to be honest, but that's
clearly an improvement).
Another test that could be added is about nextval() and setval() that
only work for temporary sequences in a read-only transaction:
create sequence foo;
create temp sequence footemp;
begin read only;
select nextval('footemp'); -- ok
select nextval('foo'); -- error
rollback;
begin read only;
select setval('footemp', 1); -- ok
select setval('foo', 1); -- error
rollback
But it is a bit funky I agree.
--
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] Improvements in psql hooks for variables
Hello,
Please consider following comments on the patch.
In function ParseVariableNum,
> if (!val || !val[0])
> return false;
Check for 'val == NULL' as in above condition is done even in callers of
ParseVariableNum().
There should be only one such check.
>+ psql_error("Invalid value \"%s\" for \"%s\"\nAn integer is
expected\n",
Cant we have this error in ParseVariableNum() similar to
ParseVariableBool() ?
>+ /*
>+* Switching AUTOCOMMIT from OFF to ON is not allowed when inside a
>+* transaction, because it cannot be effective until the current
>+* transaction is ended.
>+*/
>+ if (autocommit && !pset.autocommit &&
>+ pset.db && PQtransactionStatus(pset.db) == PQTRANS_INTRANS)
>+ {
>+ psql_error("cannot set AUTOCOMMIT to %s when inside a
transaction\n", newval);
>+ }
The above change in autocommit behaviour needs to be documented.
Thank you,
Rahila Syed
On Wed, Jan 25, 2017 at 5:48 PM, Rahila Syed wrote:
> Hello,
>
> The patch works fine on applying on latest master branch and testing it
> for various variables as listed in PsqlSettings struct.
> I will post further comments on patch soon.
>
> Thank you,
> Rahila Syed
>
> On Wed, Jan 25, 2017 at 1:33 AM, Tom Lane wrote:
>
>> "Daniel Verite" writes:
>> > Here's an update with these changes:
>>
>> I scanned this patch very quickly and think it addresses my previous
>> stylistic objections. Rahila is the reviewer of record though, so
>> I'll wait for his comments before going further.
>>
>> regards, tom lane
>>
>
>
Re: [HACKERS] Create a separate test file for exercising system views
On Mon, Jan 30, 2017 at 6:02 AM, Tom Lane wrote: > In connection with the "pg_hba_file_settings view patch" thread, I was > wondering where we could logically insert a regression test case for that > view. I realized that there is no natural home for it among the existing > regression tests, because it's not really connected to any SQL language > feature. The same is true for a number of other built-in views, and > unsurprisingly, most of them are not exercised anywhere :-(. > > Accordingly, I propose creating a new regression test file whose charter > is to exercise the SRFs underlying system views, as per attached. > I don't think we desperately need new tests for views that expand to > simple SQL, but these test cases correspond directly to code coverage > for C functions, so they seem worthwhile. > > I did not do anything about testing the various pg_stat_xxx views. > Those could be added later, or maybe they deserve their own home. > (In many cases, those would need something smarter than the basic > count(*) technique used here, because the C functions are invoked > in the view's SELECT list not in FROM, so the planner would throw > away those calls.) > > Comments/objections? Nice idea to group those tests in the same file. I am not noticing any issues with the patch proposed. -- 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] [PATCH] Rename pg_switch_xlog to pg_switch_wal
* Andres Freund (and...@anarazel.de) wrote: > On 2017-01-28 08:47:03 +0900, Michael Paquier wrote: > > On Sat, Jan 28, 2017 at 8:03 AM, Peter Eisentraut > > wrote: > > > On 1/26/17 2:05 PM, Robert Haas wrote: > > >> I do not think it can be right to rename the directory and not > > >> anything else. > > > > > > I think this is the root of the confusion. > > > > > > A lot of people apparently consented to renaming pg_xlog with the > > > understanding that that's it, whereas other people understood it as > > > consensus for renaming everything. > > > > > > You really have (at least) three options here: > > > > > > 1. Rename nothing > > > 2. Rename directory only > > > 3. Rename everything > > > > > > and you need to count the votes of each pair separately. > > > > 4. Rename everything with aliases. > > > > I would vote for 4., or 3. for consistency if 4. is not an eligible choice. > > 2, 4, 1, 3 For me, it's: 3, 4, 1, 2. I tend to agree with the other discussion, which points out that aliases for everything isn't really practical.. Of course, I'd also like to drop things like pg_user and pg_shadow one day... Never thought they'd last anywhere near this long when I first wrote those views, 12 years ago... Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Superowners
Jim, * Jim Nasby (jim.na...@bluetreble.com) wrote: > On 1/29/17 4:44 PM, Stephen Frost wrote: > >* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote: > >>On 1/26/17 1:25 PM, Simon Riggs wrote: > >>>That should include the ability to dump all objects, yet without any > >>>security details. And it should allow someone to setup logical > >>>replication easily, including both trigger based and new logical > >>>replication. And GRANT ON ALL should work. > >>This basically sounds like a GRANT $privilege ON ALL $objecttype TO > >>$user. So you could have a user that can read everything, for example. > >> > >>This kind of thing has been asked for many times, but that quieted down > >>when the default privileges feature appeared. I think it would still be > >>useful. > >Agreed. I would think we'd either do this with a default role or a role > >attribute. > > Someone was asking for that on Slack the other day, because their > customer wanted it. Default privs would not fit the bill: they > wanted to grant specific roles the ability to read everything in the > database (or maybe cluster; I don't think the conversation got into > that level of detail). ... eh? If we create a default role called "pg_read_only" which admins can grant to whomever they wish, how does that not "fit the bill"? For my 2c, at least, evaluating the various requests and coming up with some set of default roles and then implementing them would be a good GSoC project.. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Transactions involving multiple postgres foreign servers
On Sat, Jan 28, 2017 at 8:41 PM, Peter Eisentraut wrote: > On 1/26/17 4:49 AM, Masahiko Sawada wrote: >> Sorry, I attached wrong version patch of pg_fdw_xact_resovler. Please >> use attached patch. > > So in some other thread we are talking about renaming "xlog", because > nobody knows what the "x" means. In the spirit of that, let's find > better names for new functions as well. It's common in English (not just the database jargon) to abbreviate "trans" by "x" [1]. xlog went a bit far by abbreviating whole "transaction" by "x". But here "xact" means "transact", which is fine. May be we should use 'X' instead of 'x', I don't know. Said that, I am fine with any other name which conveys what the function does. [1] https://en.wikipedia.org/wiki/X -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database 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] Superowners
On 1/29/17 4:44 PM, Stephen Frost wrote: * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote: On 1/26/17 1:25 PM, Simon Riggs wrote: That should include the ability to dump all objects, yet without any security details. And it should allow someone to setup logical replication easily, including both trigger based and new logical replication. And GRANT ON ALL should work. This basically sounds like a GRANT $privilege ON ALL $objecttype TO $user. So you could have a user that can read everything, for example. This kind of thing has been asked for many times, but that quieted down when the default privileges feature appeared. I think it would still be useful. Agreed. I would think we'd either do this with a default role or a role attribute. Someone was asking for that on Slack the other day, because their customer wanted it. Default privs would not fit the bill: they wanted to grant specific roles the ability to read everything in the database (or maybe cluster; I don't think the conversation got into that level of detail). -- 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: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
On 1/29/17 2:35 AM, Fabien COELHO wrote: I'm wondering what pg would do on "EXISTS(SELECT 1 FROM customer)" if there are many employees. [...] I believe that the scan stops on the first row it finds, because the EXITS() clause is met. Hmmm... That is not so clear from "EXPLAIN" output: You need to use a better test case... explain analyze select exists(select 1 from generate_series(1,9) gs); QUERY PLAN --- Result (cost=0.01..0.02 rows=1 width=1) (actual time=26.278..26.278 rows=1 loops=1) InitPlan 1 (returns $0) -> Function Scan on generate_series gs (cost=0.00..10.00 rows=1000 width=0) (actual time=26.271..26.271 rows=1 loops=1) Planning time: 6.568 ms Execution time: 48.917 ms (5 rows) In any case, +1 for not promoting count(*) <> 0; that's a really, really bad way to test for existence. -- 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] pg_hba_file_settings view patch
On Sun, Jan 29, 2017 at 9:18 AM, Tom Lane wrote: > I wrote: > > I spent awhile hacking on this, and made a lot of things better, but > > I'm still very unhappy about the state of the comments. > > I made another pass over this, working on the comments and the docs, > and changing the view name to "pg_hba_file_rules". I think this version > is committable if people are satisfied with that name. > Thanks for working on the patch. I am fine with the "pg_hba_file_rules" name. I have to improve in writing better comments after checking the attached patch. I will improve the comments in further patch submissions to community. > One loose end is what to do about testing. I did not much like the > proposed TAP tests. We could just put "select count(*) > 0 from > pg_hba_file_rules" into the main regression tests, which would provide > some code coverage there, if not very much guarantee that what the view > outputs is sane. > I added the test in main regression test to the patch which you shared based on the mail of creating separate tests for system views in [1]. The attached needs to be applied on top the patch shared in [1]. [1] - https://www.postgresql.org/message-id/19359.1485723741%40sss.pgh.pa.us Regards, Hari Babu Fujitsu Australia pg_hba_rules_16.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] Supporting huge pages on Windows
From: Amit Kapila [mailto:amit.kapil...@gmail.com] > Hmm. It doesn't work even on a command prompt with administrative > privileges. It gives below error: > > waiting for server to start2017-01-17 11:20:13.780 IST [4788] FATAL: > could not create shared memory segment: error code 1450 > 2017-01-17 11:20:13.780 IST [4788] DETAIL: Failed system call was > CreateFileMap ping(size=148897792, > name=Global/PostgreSQL:E:/WorkSpace/PostgreSQL/master/Data) > . > 2017-01-17 11:20:13.780 IST [4788] LOG: database system is shut down > stopped waiting > pg_ctl: could not start server > Examine the log output. > > > Now, error code 1450 can occur due to insufficient system resources, so > I have tried by increasing the size of shared memory (higher value of > shared_buffers) without your patch and it works. This indicates some > problem with the patch. Hmm, the large-page requires contiguous memory for each page, so this error could occur on a long-running system where the memory is heavily fragmented. For example, please see the following page and check the memory with RAMMap program referred there. http://blog.dbi-services.com/large-pages-and-memory_target-on-windows/ BTW, is your OS or PostgreSQL 32-bit? > > It seems that Windows removes many privileges, including "Lock Pages > in Memory", when starting the normal command prompt. As its evidence, you > can use the attached priv.c to see what privileges are assigned and and > enabled/disabled. Build it like "cl priv.c" and just run priv.exe on each > command prompt. Those runs show different privileges. > > > > This is bad. > > > Should I need to do something, e.g. explain in the document that the user > should use the command prompt with administrative privileges when he uses > huge_pages? > > > > I think it is better to document in some way if we decide to go-ahead with > the patch. Sure, I added these sentences. +To start the database server on the command prompt as a standalone process, +not as a Windows service, run the command prompt as an administrator or +disable the User Access Control (UAC). When the UAC is enabled, the normal +command prompt revokes the user right Lock Pages in Memory. Regards Takayuki Tsunakawa win_large_pages_v7.patch Description: win_large_pages_v7.patch -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Transactions involving multiple postgres foreign servers
On 2017/01/29 0:11, Peter Eisentraut wrote: On 1/26/17 4:49 AM, Masahiko Sawada wrote: Sorry, I attached wrong version patch of pg_fdw_xact_resovler. Please use attached patch. So in some other thread we are talking about renaming "xlog", because nobody knows what the "x" means. In the spirit of that, let's find better names for new functions as well. +1 Regards, Vinayak Pokale 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] [PATCH]: fix bug in SP-GiST box_ops
Working on the tests for new SP-GiST opclasses for polygons and circles, I've
found a bug in the SP-GiST box_ops (added in 9.6): some operators
(&<, &>, $<|, |&>) have wrong tests in spg_box_quad_inner_consistent().
This obviously leads to incorrect results of a SP-GiST index scan (see tests
in the attached patch, their results were taken from a sequential scan).
--
Nikita Glukhov
Postgres Professional:http://www.postgrespro.com
The Russian Postgres Company
diff --git a/src/backend/utils/adt/geo_spgist.c b/src/backend/utils/adt/geo_spgist.c
index aacb340..446aa38 100644
--- a/src/backend/utils/adt/geo_spgist.c
+++ b/src/backend/utils/adt/geo_spgist.c
@@ -280,74 +280,90 @@ contained4D(RectBox *rect_box, RangeBox *query)
/* Can any range from range_box to be lower than this argument? */
static bool
-lower2D(RangeBox *range_box, Range *query)
+lower2D(RangeBox *range_box, double query)
{
- return FPlt(range_box->left.low, query->low) &&
- FPlt(range_box->right.low, query->low);
+ return FPlt(range_box->left.low, query) &&
+ FPlt(range_box->right.low, query);
+}
+
+/* Can any range from range_box to be lower or equal to this argument? */
+static bool
+lowerEqual2D(RangeBox *range_box, double query)
+{
+ return FPle(range_box->left.low, query) &&
+ FPle(range_box->right.low, query);
}
/* Can any range from range_box to be higher than this argument? */
static bool
-higher2D(RangeBox *range_box, Range *query)
+higher2D(RangeBox *range_box, double query)
+{
+ return FPgt(range_box->left.high, query) &&
+ FPgt(range_box->right.high, query);
+}
+
+/* Can any range from range_box to be higher or equal to this argument? */
+static bool
+higherEqual2D(RangeBox *range_box, double query)
{
- return FPgt(range_box->left.high, query->high) &&
- FPgt(range_box->right.high, query->high);
+ return FPge(range_box->left.high, query) &&
+ FPge(range_box->right.high, query);
}
/* Can any rectangle from rect_box be left of this argument? */
static bool
left4D(RectBox *rect_box, RangeBox *query)
{
- return lower2D(&rect_box->range_box_x, &query->left);
+ return lower2D(&rect_box->range_box_x, query->left.low);
}
/* Can any rectangle from rect_box does not extend the right of this argument? */
static bool
overLeft4D(RectBox *rect_box, RangeBox *query)
{
- return lower2D(&rect_box->range_box_x, &query->right);
+ return lowerEqual2D(&rect_box->range_box_x, query->left.high);
}
/* Can any rectangle from rect_box be right of this argument? */
static bool
right4D(RectBox *rect_box, RangeBox *query)
{
- return higher2D(&rect_box->range_box_x, &query->left);
+ return higher2D(&rect_box->range_box_x, query->left.high);
}
/* Can any rectangle from rect_box does not extend the left of this argument? */
static bool
overRight4D(RectBox *rect_box, RangeBox *query)
{
- return higher2D(&rect_box->range_box_x, &query->right);
+ return higherEqual2D(&rect_box->range_box_x, query->left.low);
}
/* Can any rectangle from rect_box be below of this argument? */
static bool
below4D(RectBox *rect_box, RangeBox *query)
{
- return lower2D(&rect_box->range_box_y, &query->right);
+ return lower2D(&rect_box->range_box_y, query->right.low);
}
/* Can any rectangle from rect_box does not extend above this argument? */
static bool
overBelow4D(RectBox *rect_box, RangeBox *query)
{
- return lower2D(&rect_box->range_box_y, &query->left);
+ return lowerEqual2D(&rect_box->range_box_y, query->right.high);
}
/* Can any rectangle from rect_box be above of this argument? */
static bool
above4D(RectBox *rect_box, RangeBox *query)
{
- return higher2D(&rect_box->range_box_y, &query->right);
+ return higher2D(&rect_box->range_box_y, query->right.high);
}
/* Can any rectangle from rect_box does not extend below of this argument? */
static bool
overAbove4D(RectBox *rect_box, RangeBox *query)
{
- return higher2D(&rect_box->range_box_y, &query->right);
+ return higherEqual2D(&rect_box->range_box_y, query->right.low);
}
/*
diff --git a/src/test/regress/expected/box.out b/src/test/regress/expected/box.out
index 5f8b945..09db8e8 100644
--- a/src/test/regress/expected/box.out
+++ b/src/test/regress/expected/box.out
@@ -455,3 +455,116 @@ EXPLAIN (COSTS OFF) SELECT * FROM box_temp WHERE f1 ~= '(20,20),(40,40)';
RESET enable_seqscan;
DROP INDEX box_spgist;
+--
+-- Test the SP-GiST index with big random data
+--
+CREATE TEMPORARY TABLE quad_box_tbl (id int, b box);
+SELECT setseed(0);
+ setseed
+-
+
+(1 row)
+
+INSERT INTO quad_box_tbl
+ SELECT i, box(point(x, y), point(x + w, y + h))
+ FROM (SELECT i,
+random() * 1000 as x, random() * 1000 as y,
+random() * 20 as w, random() * 20 as h
+ FROM generate_series(1, 1) AS i) q;
+-- Insert repeating data to test allTheSame
+INSERT INTO quad_box_tbl
+ SELECT i, '((200, 300),(210, 310))'
+ FROM generate_series(10001, 11000) AS i;
+INSERT INTO quad_box_tbl
+ VALUES
+ (11001, NULL),
+ (11002, NULL),
+ (11003, '((-infinity,-infinity),(infini
Re: [HACKERS] Superowners
* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote: > On 1/26/17 1:25 PM, Simon Riggs wrote: > > That should include the ability to dump all objects, yet without any > > security details. And it should allow someone to setup logical > > replication easily, including both trigger based and new logical > > replication. And GRANT ON ALL should work. > > This basically sounds like a GRANT $privilege ON ALL $objecttype TO > $user. So you could have a user that can read everything, for example. > > This kind of thing has been asked for many times, but that quieted down > when the default privileges feature appeared. I think it would still be > useful. Agreed. I would think we'd either do this with a default role or a role attribute. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal
On 2017-01-28 08:47:03 +0900, Michael Paquier wrote: > On Sat, Jan 28, 2017 at 8:03 AM, Peter Eisentraut > wrote: > > On 1/26/17 2:05 PM, Robert Haas wrote: > >> I do not think it can be right to rename the directory and not > >> anything else. > > > > I think this is the root of the confusion. > > > > A lot of people apparently consented to renaming pg_xlog with the > > understanding that that's it, whereas other people understood it as > > consensus for renaming everything. > > > > You really have (at least) three options here: > > > > 1. Rename nothing > > 2. Rename directory only > > 3. Rename everything > > > > and you need to count the votes of each pair separately. > > 4. Rename everything with aliases. > > I would vote for 4., or 3. for consistency if 4. is not an eligible choice. 2, 4, 1, 3 -- 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] Rename pg_switch_xlog to pg_switch_wal
On Sat, Jan 28, 2017 at 12:43 PM, David Steele wrote: > The problem I have with aliases is that they would need to be done across > the board. At the least, we would need function aliases, symlinks for the > binaries (which would rneed to be done by the packagers), aliases for the > command line options, and lots of notations in the documentation. I was not precise enough here, I am fine with only the function aliases, because they just have as cost an extra line in pg_proc.h and because binary-level aliases on Windows are a pain. > All of this would need to be preserved more or less indefinitely, because if > we can't decide on a break now it's not likely to happen later. That's true as well. -- 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_ls_dir & friends still have a hard-coded superuser check
On Sun, Jan 29, 2017 at 05:52:51PM -0500, Robert Haas wrote: > On Sun, Jan 29, 2017 at 5:39 PM, David Fetter wrote: > > On Thu, Jan 26, 2017 at 08:50:27AM -0500, Robert Haas wrote: > >> On Wed, Jan 25, 2017 at 10:31 PM, Stephen Frost wrote: > >> > Frankly, I get quite tired of the argument essentially being made > >> > here that because pg_ls_dir() wouldn't grant someone superuser > >> > rights, that we should remove superuser checks from everything. > >> > As long as you are presenting it like that, I'm going to be quite > >> > dead-set against any of it. > >> 1. pg_ls_dir. I cannot see how this can ever be used to get > >> superuser privileges. > > > > With pilot error, all things are possible. A file name under $PGDATA > > could be the superuser password. > > Uh, true. The default value of application_name could be the > superuser password, too, but we still allow access to it by > unprivileged users. Of course. 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] pg_ls_dir & friends still have a hard-coded superuser check
On Sun, Jan 29, 2017 at 5:39 PM, David Fetter wrote: > On Thu, Jan 26, 2017 at 08:50:27AM -0500, Robert Haas wrote: >> On Wed, Jan 25, 2017 at 10:31 PM, Stephen Frost wrote: >> > Frankly, I get quite tired of the argument essentially being made >> > here that because pg_ls_dir() wouldn't grant someone superuser >> > rights, that we should remove superuser checks from everything. >> > As long as you are presenting it like that, I'm going to be quite >> > dead-set against any of it. >> 1. pg_ls_dir. I cannot see how this can ever be used to get >> superuser privileges. > > With pilot error, all things are possible. A file name under $PGDATA > could be the superuser password. Uh, true. The default value of application_name could be the superuser password, too, but we still allow access to it by unprivileged users. -- 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] pg_ls_dir & friends still have a hard-coded superuser check
On Thu, Jan 26, 2017 at 08:50:27AM -0500, Robert Haas wrote: > On Wed, Jan 25, 2017 at 10:31 PM, Stephen Frost wrote: > > Frankly, I get quite tired of the argument essentially being made > > here that because pg_ls_dir() wouldn't grant someone superuser > > rights, that we should remove superuser checks from everything. > > As long as you are presenting it like that, I'm going to be quite > > dead-set against any of it. > 1. pg_ls_dir. I cannot see how this can ever be used to get > superuser privileges. With pilot error, all things are possible. A file name under $PGDATA could be the superuser password. 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] Performance improvement for joins where outer side is unique
On 28 January 2017 at 05:04, Tom Lane wrote: > David Rowley writes: >> I agree that special handling of one join type is not so pretty. >> However, LEFT JOINs still remain a bit special as they're the only >> ones we currently perform join removal on, and the patch modifies that >> code to make use of the new flag for those. This can improve planner >> performance of join removal when a join is removed successfully, as >> the previous code had to recheck uniqueness of each remaining LEFT >> JOIN again, whereas the new code only checks uniqueness of ones not >> previously marked as unique. This too likely could be done with the >> cache, although I'm a bit concerned with populating the cache, then >> performing a bunch of LEFT JOIN removals and leaving relids in the >> cache which no longer exist. Perhaps it's OK. I've just not found >> proofs in my head yet that it is. > > TBH, I do not like that tie-in at all. I don't believe that it improves > any performance, because if analyzejoins.c detects that the join is > unique, it will remove the join; therefore there is nothing to cache. > (This statement depends on the uniqueness test being the last removability > test, but it is.) And running mark_unique_joins() multiple times is ugly > and adds cycles whenever it cannot prove a join unique, because it'll keep > trying to do so. So I'm pretty inclined to drop the connection to > analyzejoins.c altogether, along with mark_unique_joins(), and just use > the generic positive/negative cache mechanism you added for all join types. I can make this change, but before I do I just want to point that I don't think what you've said here is entirely accurate. Let's assume unique joins are very common place, and join removals are not so common. If a query has 5 left joins, and only one of which can be removed, then the new code will most likely perform 5 unique join checks, whereas the old code would perform 9, as those unique checks are performed again once the 1 relation is removed for the remaining 4. However I'll go make the change as something needs fixed in that area anyway, as LEFT JOINs use the additional quals, whereas other join types don't, which is broken. -- David Rowley 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] Create a separate test file for exercising system views
Hi, On 2017-01-29 16:02:21 -0500, Tom Lane wrote: > I did not do anything about testing the various pg_stat_xxx views. > Those could be added later, or maybe they deserve their own home. > (In many cases, those would need something smarter than the basic > count(*) technique used here, because the C functions are invoked > in the view's SELECT list not in FROM, so the planner would throw > away those calls.) I've previously wished there were a portable equivalent of \o /dev/null that'd not be perfect, but it'd still exercise more than what we currently have. Alternatively casting the entire row to text should allow to use count(*) trickery in some of the cases at least. > Comments/objections? Sounds like a good idea here. Greetings, Andres Freund -- 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] CONNECTION LIMIT and Parallel Query don't play well together
On 27 January 2017 at 03:53, Robert Haas wrote: > Sorry, this had slipped through the cracks -- I'm having a very hard > time keeping up with the flow of patches and emails. But it looks > good to me, except that it seems like CountDBBackends() needs the same > fix (and probably a corresponding documentation update). Thanks for looking at this. Looks like there's a few other usages of CountDBBackends() which require background workers to be counted too, so I ended up creating CountDBConnections() as I didn't really think adding a bool flag to CountDBBackends was so nice. I thought about renaming CountUserBackends() to become CountUserConnections(), but I've not. Although, perhaps its better to break any third party stuff that uses that so that authors can review which behaviour they need rather than have their extension silently break? David -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services connection_limit_ignore_bgworkers_v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Create a separate test file for exercising system views
In connection with the "pg_hba_file_settings view patch" thread, I was wondering where we could logically insert a regression test case for that view. I realized that there is no natural home for it among the existing regression tests, because it's not really connected to any SQL language feature. The same is true for a number of other built-in views, and unsurprisingly, most of them are not exercised anywhere :-(. Accordingly, I propose creating a new regression test file whose charter is to exercise the SRFs underlying system views, as per attached. I don't think we desperately need new tests for views that expand to simple SQL, but these test cases correspond directly to code coverage for C functions, so they seem worthwhile. I did not do anything about testing the various pg_stat_xxx views. Those could be added later, or maybe they deserve their own home. (In many cases, those would need something smarter than the basic count(*) technique used here, because the C functions are invoked in the view's SELECT list not in FROM, so the planner would throw away those calls.) Comments/objections? regards, tom lane diff --git a/src/test/regress/expected/rangefuncs.out b/src/test/regress/expected/rangefuncs.out index 56481de..526a4ae 100644 *** a/src/test/regress/expected/rangefuncs.out --- b/src/test/regress/expected/rangefuncs.out *** *** 1,19 - SELECT name, setting FROM pg_settings WHERE name LIKE 'enable%'; - name | setting - --+- - enable_bitmapscan| on - enable_hashagg | on - enable_hashjoin | on - enable_indexonlyscan | on - enable_indexscan | on - enable_material | on - enable_mergejoin | on - enable_nestloop | on - enable_seqscan | on - enable_sort | on - enable_tidscan | on - (11 rows) - CREATE TABLE foo2(fooid int, f2 int); INSERT INTO foo2 VALUES(1, 11); INSERT INTO foo2 VALUES(2, 22); --- 1,3 diff --git a/src/test/regress/expected/sysviews.out b/src/test/regress/expected/sysviews.out index ...852a7c3 . *** a/src/test/regress/expected/sysviews.out --- b/src/test/regress/expected/sysviews.out *** *** 0 --- 1,113 + -- + -- Test assorted system views + -- + -- This test is mainly meant to provide some code coverage for the + -- set-returning functions that underlie certain system views. + -- The output of most of these functions is very environment-dependent, + -- so our ability to test with fixed expected output is pretty limited; + -- but even a trivial check of count(*) will exercise the normal code path + -- through the SRF. + select count(*) >= 0 as ok from pg_available_extension_versions; + ok + + t + (1 row) + + select count(*) >= 0 as ok from pg_available_extensions; + ok + + t + (1 row) + + -- At introduction, pg_config had 23 entries; it may grow + select count(*) > 20 as ok from pg_config; + ok + + t + (1 row) + + -- We expect no cursors in this test; see also portals.sql + select count(*) = 0 as ok from pg_cursors; + ok + + t + (1 row) + + select count(*) >= 0 as ok from pg_file_settings; + ok + + t + (1 row) + + -- There will surely be at least one active lock + select count(*) > 0 as ok from pg_locks; + ok + + t + (1 row) + + -- We expect no prepared statements in this test; see also prepare.sql + select count(*) = 0 as ok from pg_prepared_statements; + ok + + t + (1 row) + + -- See also prepared_xacts.sql + select count(*) >= 0 as ok from pg_prepared_xacts; + ok + + t + (1 row) + + -- This is to record the prevailing planner enable_foo settings during + -- a regression test run. + select name, setting from pg_settings where name like 'enable%'; + name | setting + --+- + enable_bitmapscan| on + enable_hashagg | on + enable_hashjoin | on + enable_indexonlyscan | on + enable_indexscan | on + enable_material | on + enable_mergejoin | on + enable_nestloop | on + enable_seqscan | on + enable_sort | on + enable_tidscan | on + (11 rows) + + -- Test that the pg_timezone_names and pg_timezone_abbrevs views are + -- more-or-less working. We can't test their contents in any great detail + -- without the outputs changing anytime IANA updates the underlying data, + -- but it seems reasonable to expect at least one entry per major meridian. + -- (At the time of writing, the actual counts are around 38 because of + -- zones using fractional GMT offsets, so this is a pretty loose test.) + select count(distinct utc_offset) >= 24 as ok from pg_timezone_names; + ok + + t + (1 row) + + select count(distinct utc_offset) >= 24 as ok from pg_timezone_abbrevs; + ok + + t + (1 row) + + -- Let's check the non-default timezone abbreviation sets, too + set timezone_abbreviations = 'Australia'; + select count(distinct utc_offset)
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
> > My point is that examples about one thing can be interpreted as example > for other things which is also done in the example, so it is better to do > everything right. > Fair enough. I'll rewrite the examples to use pk lookups. I doubt the query plan for those will change much in the future. > Hmmm. Copy-pasting is bad enough, and "when the other patch is committed" > is an unknown, so I would still suggest to fix obvious defects at least (eg > dead code which may result in compiler warnings, inconsistent comments...). > It was do that or pause this work until that unknown was resolved. > > My point was that you must at least push something, otherwise both > branches are executed (!), and some commands could be attached to > upper-level conditions: > > > As for which state is pushed, it is indeed debatable. I do think that > pushing ignore on errors is a better/less risky behavior, but other people' > opinion may differ. +1 > > > On "else" when in state ignored, ISTM that it should remain in state >>> ignore, not switch to else-false. >>> >> >> That's how I know if this is the first "else" I encountered. >> > > Ok, my mistake. Maybe expand the comment a little bit if appropriate. +1 > > As I recall, history is only for interactive mode. If I really typed > something, I'm expecting to get it by visiting previous commands, because I > certainly do not want to retype it again. > > For your above example, maybe I would reedit the clause definition, then > want to execute the delete. Good points, and history does save the string with the variable in it, not the resolved string that was sent (or not sent) to the server.
Re: [HACKERS] Should buffer of initialization fork have a BM_PERMANENT flag
On Thu, Jan 26, 2017 at 9:14 AM, Michael Paquier
wrote:
> So the patch attached fixes the problem by changing BufferAlloc() in
> such a way that initialization forks are permanently written to disk,
> which is what you are suggesting. As a simple fix for back-branches
> that's enough, though on HEAD I think that we should really rework the
> empty() routines so as the write goes through shared buffers first,
> that seems more solid than relying on the sgmr routines to do this
> work. Robert, what do you think?
Attached is what I have in mind for HEAD. btree, gist, spgist and
bloom indexes are changed so as the init forks created go through the
shared buffers instead of having their empty() routines handle the
flush of the page created. This removes any kind of race conditions
between the checkpointer and the init fork creations, which is I think
a good thing.
Here are the tests I have done.
First running those commands to create all types of indexes.
create extension bloom;
create extension btree_gist;
create extension btree_gin;
create unlogged table foo (a int);
create index foo_bt on foo(a);
create index foo_bloom on foo using bloom(a);
create index foo_gin on foo using gin (a);
create index foo_gist on foo using gist (a);
create index foo_brin on foo using brin (a);
create unlogged table foo_geo (a box);
create index foo_spgist ON foo_geo using spgist(a);
checkpoint;
Then crash the server, restart it, and the following vacuums are able
to complete.
vacuum foo;
vacuum foo_geo;
I have as well created a CF entry for this set of patches:
https://commitfest.postgresql.org/13/971/
Thanks,
--
Michael
diff --git a/contrib/bloom/blinsert.c b/contrib/bloom/blinsert.c
index 29bc5cea79..d04298add5 100644
--- a/contrib/bloom/blinsert.c
+++ b/contrib/bloom/blinsert.c
@@ -158,31 +158,24 @@ blbuild(Relation heap, Relation index, IndexInfo *indexInfo)
void
blbuildempty(Relation index)
{
- Page metapage;
+ Buffer MetaBuffer;
- /* Construct metapage. */
- metapage = (Page) palloc(BLCKSZ);
- BloomFillMetapage(index, metapage);
+ /* An empty bloom index has one meta page */
+ MetaBuffer =
+ ReadBufferExtended(index, INIT_FORKNUM, P_NEW, RBM_NORMAL, NULL);
+ LockBuffer(MetaBuffer, BUFFER_LOCK_EXCLUSIVE);
- /*
- * Write the page and log it. It might seem that an immediate sync
- * would be sufficient to guarantee that the file exists on disk, but
- * recovery itself might remove it while replaying, for example, an
- * XLOG_DBASE_CREATE or XLOG_TBLSPC_CREATE record. Therefore, we
- * need this even when wal_level=minimal.
- */
- PageSetChecksumInplace(metapage, BLOOM_METAPAGE_BLKNO);
- smgrwrite(index->rd_smgr, INIT_FORKNUM, BLOOM_METAPAGE_BLKNO,
- (char *) metapage, true);
- log_newpage(&index->rd_smgr->smgr_rnode.node, INIT_FORKNUM,
-BLOOM_METAPAGE_BLKNO, metapage, false);
+ /* Initialize the meta buffer */
+ BloomFillMetapage(index, BufferGetPage(MetaBuffer));
- /*
- * An immediate sync is required even if we xlog'd the page, because the
- * write did not go through shared_buffers and therefore a concurrent
- * checkpoint may have moved the redo pointer past our xlog record.
- */
- smgrimmedsync(index->rd_smgr, INIT_FORKNUM);
+ /* And log it */
+ START_CRIT_SECTION();
+ MarkBufferDirty(MetaBuffer);
+ log_newpage_buffer(MetaBuffer, false);
+ END_CRIT_SECTION();
+
+ /* Unlock and release the buffer */
+ UnlockReleaseBuffer(MetaBuffer);
}
/*
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 1bb1acfea6..215d3c6c02 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -23,6 +23,7 @@
#include "access/xlog.h"
#include "catalog/index.h"
#include "commands/vacuum.h"
+#include "miscadmin.h"
#include "storage/indexfsm.h"
#include "storage/ipc.h"
#include "storage/lmgr.h"
@@ -237,31 +238,22 @@ btbuildCallback(Relation index,
void
btbuildempty(Relation index)
{
- Page metapage;
-
- /* Construct metapage. */
- metapage = (Page) palloc(BLCKSZ);
- _bt_initmetapage(metapage, P_NONE, 0);
-
- /*
- * Write the page and log it. It might seem that an immediate sync
- * would be sufficient to guarantee that the file exists on disk, but
- * recovery itself might remove it while replaying, for example, an
- * XLOG_DBASE_CREATE or XLOG_TBLSPC_CREATE record. Therefore, we
- * need this even when wal_level=minimal.
- */
- PageSetChecksumInplace(metapage, BTREE_METAPAGE);
- smgrwrite(index->rd_smgr, INIT_FORKNUM, BTREE_METAPAGE,
- (char *) metapage, true);
- log_newpage(&index->rd_smgr->smgr_rnode.node, INIT_FORKNUM,
-BTREE_METAPAGE, metapage, false);
-
- /*
- * An immediate sync is required even if we xlog'd the page, because the
- * write did not go through shared_buffers and therefore a concurrent
- * checkpoint may have moved the redo pointer past our xlog record.
- */
- smgrimmedsync(index->rd_smgr, INIT_FORKNUM);
+ Buffer metabuffer;
+
+ /* An empty btree index has one meta page */
+ metabuffer
Re: [HACKERS] WIP: [[Parallel] Shared] Hash
On Sat, Jan 7, 2017 at 9:01 AM, Thomas Munro wrote: > Stepping back a bit, I am aware of the following approaches to hash > join parallelism: > > 1. Run the inner plan and build a private hash table in each > participant [...]. > > 2. Run a partition-wise hash join[1]. [...] > > 3. Repartition the data on the fly, and then run a partition-wise > hash join. [...] > > 4. Scatter both the inner and outer plans arbitrarily across > participants [...], and build a shared hash > table. [...] > > [...] I suspect that 4 is probably a better > fit than 3 for Postgres today, because the communication overhead of > shovelling nearly all tuples through extra tuple queues to route them > to the right hash table would surely be very high, though I can see > that it's very attractive to have a reusable tuple repartitioning > operator and then run k disjoint communication-free joins (again, > without code change to the join operator, and to the benefit of all > join operators). On this topic, I recently stumbled on the 2011 paper "Design and Evaluation of Main Memory Hash Join Algorithms for Multi-core CPUs"[1] and found it reassuring. It compares simple shared hash tables to some state-of-the-art repartitioning approaches (including the radix join algorithm which performs the amazing feat of building a lot of cacheline-sized hash tables and then runs with minimal cache misses). From the introduction: "Second, we show that an algorithm that does not do any partitioning, but simply constructs a single shared hash table on the build relation often outperforms more complex algorithms. This simple “no-partitioning” hash join algorithm is robust to sub-optimal parameter choices by the optimizer, and does not require any knowledge of the characteristics of the input to work well. To the best of our knowledge, this simple hash join technique differs from what is currently implemented in existing DBMSs for multi-core hash join processing, and offers a tantalizingly simple, efficient, and robust technique for implementing the hash join operation." "Finally, we show that the simple “no-partitioning” hash join algorithm takes advantage of intrinsic hardware optimizations to handle skew. As a result, this simple hash join technique often benefits from skew and its relative performance increases as the skew increases! This property is a big advancement over the state-of-the-art methods, as it is important to have methods that can gracefully handle skew in practice [8]." (That relates to SMT pipelining compensating for the extra cacheline misses during probing by doing thread A's work while waiting for thread B's memory to be fetched.) From the conclusion: "... Our results show that a simple hash join technique that does not do any partitioning of the input relations often outperforms the other more complex partitioning-based join alternatives. In addition, the relative performance of this simple hash join technique rapidly improves with increasing skew, and it outperforms every other algorithm in the presence of even small amounts of skew." For balance, the authors of a 2013 paper "Main-Memory Hash Joins on Multi-Core CPUs: Tuning to the Underlying Hardware"[2] are less keen on the simple "hardware-oblivious" "no partitioning" approach and don't buy the other paper's ideas about SMT. Incidentally, their results on the benefits of large (huge) pages are interesting (table VI) and suggest that huge page support for DSM segments could be good here. [1] https://pdfs.semanticscholar.org/9de4/b32f2c7b630a4f6aae6994a362a46c7c49e9.pdf [2] https://www.inf.ethz.ch/personal/cagri.balkesen/publications/parallel-joins-icde13.pdf -- Thomas Munro 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] Cache Hash Index meta page.
> HashMetaPage _hash_getcachedmetap(Relation rel, Buffer *metabuf, bool > force_refresh); > > If the cache is initialized and force_refresh is not true, then this > just returns the cached data, and the metabuf argument isn't used. > Otherwise, if *metabuf == InvalidBuffer, we set *metabuf to point to > the metabuffer, pin and lock it, use it to set the cache, release the > lock, and return with the pin still held. If *metabuf != > InvalidBuffer, we assume it's pinned and return with it still pinned. Thanks, Robert I have made a new patch which tries to do same. Now I think code looks less complicated. -- Thanks and Regards Mithun C Y EnterpriseDB: http://www.enterprisedb.com cache_hash_index_meta_page_14.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: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
Hello, I'm wondering what pg would do on "EXISTS(SELECT 1 FROM customer)" if there are many employees. [...] I believe that the scan stops on the first row it finds, because the EXITS() clause is met. Hmmm... That is not so clear from "EXPLAIN" output: Result (cost=0.03..0.04 rows=1 width=1) InitPlan 1 (returns $0) -> Seq Scan on ... (cost=0.00..263981.69 rows=10001769 width=0) There is a plan for the sub-query, so it looks like it is actually fully executed. Maybe adding "LIMIT 1" would be better? But it's not relevant to the documentation, I simply wanted to demonstrate some results that couldn't be resolved at parse time, so that the \if tests were meaningful. If the query example is distracting from the point of the documentation, we should change it. My point is that examples about one thing can be interpreted as example for other things which is also done in the example, so it is better to do everything right. In "read_boolean_expression": [...] This is code lifted from variable.c's ParseVariableBool(). When the other patch for "psql hooks" is committed (the one that detects when the string wasn't a valid boolean), this code will go away and we'll just use ParseVariableBool() again. Hmmm. Copy-pasting is bad enough, and "when the other patch is committed" is an unknown, so I would still suggest to fix obvious defects at least (eg dead code which may result in compiler warnings, inconsistent comments...). [...] The second test on success may not rely on the value set above. That looks very strange. ISTM that the state should be pushed whether parsing succeeded or not. Moreover, it results in: \if ERROR \echo X \else \echo Y \endif having both X & Y printed and error reported on else and endif. I think that an expression error should just put the stuff in ignore state. Not just false, but ignore the whole if-endif? interesting. I hadn't thought of that. Can do. My point was that you must at least push something, otherwise both branches are executed (!), and some commands could be attached to upper-level conditions: \if true \if ERROR ... \endif // this becomes "if true \endif" ... \endif // this becomes an error As for which state is pushed, it is indeed debatable. I do think that pushing ignore on errors is a better/less risky behavior, but other people' opinion may differ. On "else" when in state ignored, ISTM that it should remain in state ignore, not switch to else-false. That's how I know if this is the first "else" I encountered. Ok, my mistake. Maybe expand the comment a little bit if appropriate. History saving: I'm wondering whether all read line should be put into history, whether executed or not. Good question. I gave it some thought and I decided it shouldn't. First, because history is a set of statements that were attempted, and those statements were not. But perhaps more importantly, because the statement could have contained an expandable variable, and since that variable would not be evaluated the SQL stored would be subtly altered from the original intent, perhaps in ways that might drastically alter the meaning of the statement. A highly contrived example: \set clause 'where cust_id = 1' \if false delete from customers :clause; \endif Hmmm. So yeah, it just seemed easier to not store in history. Hmmm. As I recall, history is only for interactive mode. If I really typed something, I'm expecting to get it by visiting previous commands, because I certainly do not want to retype it again. For your above example, maybe I would reedit the clause definition, then want to execute the delete. -- 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] pgbench - allow to store select results into variables
Please pardon the redondance: this is a slightly edited repost from another thread where motivation for this patch was discussed, so that it appear in the relevant thread. Tom> [...] there was immediately objection as to whether his idea of TPC-B Tom> compliance was actually right. From my point of view TPC-* are simply objective examples of typical benchmark requirements to show which features are needed in a tool for doing this activity. Once features are available, I think that pgbench should also be a show-case for their usage. Currently a few functions (for implementing the bench as specified) and actually extracting results into variables (for suspicious auditors and bench relevance, see below) are missing. Tom> I remember complaining that he had a totally artificial idea of what Tom> "fetching a data value" requires. Yep. I think that the key misunderstanding is that you are honest and assume that other people are honest too. This is naïve: There is a long history of vendors creatively "cheating" to get better than deserve benchmark results. Benchmark specifications try to prevent such behaviors by laying careful requirements and procedures. In this instance, you "know" that when pg has returned the result of the query the data is actually on the client side, so you considered it is fetched. That is fine for you, but from a benchmarking perspective with external auditors your belief/knowledge is not good enough. For instance, the vendor could implement a new version of the protocol where the data are only transfered on demand, and the result just tells that the data is indeed somewhere on the server (eg on "SELECT abalance" it could just check that the key exists, no need to actually fetch the data from the table, so no need to read the table, the index is enough...). That would be pretty stupid for real application performance, but the benchmark would get better tps by doing so. Without even intentionnaly cheating, this could be part of a useful "streaming mode" protocol option which make sense for very large results but would be activated for a small result. Another point is that decoding the message may be a little expensive, so that by not actually extracting the data into the client but just keeping it in the connection/OS one gets better performance. Thus, TPC-B 2.0.0 benchmark specification says: "1.3.2 Each transaction shall return to the driver the Account_Balance resulting from successful commit of the transaction. Comment: It is the intent of this clause that the account balance in the database be returned to the driver, i.e., that the application retrieve the account balance." For me the correct interpretation of "the APPLICATION retrieve the account balance" is that the client application code, pgbench in this context, did indeed get the value from the vendor code, here "libpq" which is handling the connection. Having the value discarded from libpq by calling PQclear instead of PQntuples/PQgetvalue/... skips a key part of the client code that no real application would skip. This looks strange and is not representative of real client code: as a potential auditor, because of this performance impact doubt and lack of relevance, I would not check the corresponding item in the audit check list: "11.3.1.2 Verify that transaction inputs and outputs satisfy Clause 1.3." So the benchmark implementation would not be validated. Another trivial reason to be able to actually retrieve data is that for benchmarking purpose it is very easy to want to test a scenario where you do different things based on data received, which imply that the data can be manipulated somehow on the benchmarking client side, which is currently not possible. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
