Re: [HACKERS] WIP: SCRAM authentication
On 8/11/15 5:18 PM, Robert Haas wrote: The thing we're actually debating here is whether enabling SCRAM authentication for a role should also mean disabling MD5 authentication for that same role, or whether you should be able to have two password verifiers stored for that role, one for SCRAM and the other MD5. If we allowed that, then you could turn SCRAM on for a role, and only later turn MD5 off. I think that's a bad plan because, in most scenarios, allowing two forms of authentication for the same account is strictly less secure than allowing only one. And also because it means adding a bunch of new system catalog machinery and SQL syntax. Instead, I think that, for any given role, you should get to pick the way that password authentication works for that role: either MD5, or SCRAM, but not whichever of those two the client prefers. I understand this idea, but I think it's not practical for many uses. There is no way to find out, on the server, whether all current clients would support a switch to SCRAM. Let alone all not-current clients. The only way to do such a switch would be to do the switch and then check for connection failures in the log, which is not good. It would be better if we allowed both methods side by side. Then an administrator can check in the logs which clients are using an old method and track those down without interrupting production. (Now that I think about this, to counter my point, this is very similar to the switch from crypt to md5. You couldn't turn that on until you were sure that all clients would support it. I don't remember the experience from back then, though.) Also, correct me if I'm wrong, but I believe using SCRAM would also make it harder to use the password exchange sniffed from the wire. So there might be a benefit to using SCRAM even if you have to keep old and supposedly insecure md5 hashes around for a while. -- 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] Foreign join pushdown vs EvalPlanQual
-Original Message-
From: pgsql-hackers-ow...@postgresql.org
[mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Etsuro Fujita
Sent: Wednesday, August 12, 2015 8:26 PM
To: Robert Haas; Kaigai Kouhei(海外 浩平)
Cc: PostgreSQL-development; 花田茂
Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
On 2015/08/12 7:21, Robert Haas wrote:
On Fri, Aug 7, 2015 at 3:37 AM, Kouhei Kaigai kai...@ak.jp.nec.com wrote:
I could have a discussion with Fujita-san about this topic.
Also, let me share with the discussion towards entire solution.
The primitive reason of this problem is, Scan node with scanrelid==0
represents a relation join that can involve multiple relations, thus,
its TupleDesc of the records will not fit base relations, however,
ExecScanFetch() was not updated when scanrelid==0 gets supported.
FDW/CSP on behalf of the Scan node with scanrelid==0 are responsible
to generate records according to the fdw_/custom_scan_tlist that
reflects the definition of relation join, and only FDW/CSP know how
to combine these base relations.
In addition, host-side expressions (like Plan-qual) are initialized
to reference the records generated by FDW/CSP, so the least invasive
approach is to allow FDW/CSP to have own logic to recheck, I think.
Below is the structure of ExecScanFetch().
ExecScanFetch(ScanState *node,
ExecScanAccessMtd accessMtd,
ExecScanRecheckMtd recheckMtd)
{
EState *estate = node-ps.state;
if (estate-es_epqTuple != NULL)
{
/*
* We are inside an EvalPlanQual recheck. Return the test tuple
if
* one is available, after rechecking any access-method-specific
* conditions.
*/
Index scanrelid = ((Scan *) node-ps.plan)-scanrelid;
Assert(scanrelid 0);
if (estate-es_epqTupleSet[scanrelid - 1])
{
TupleTableSlot *slot = node-ss_ScanTupleSlot;
:
return slot;
}
}
return (*accessMtd) (node);
}
When we are inside of EPQ, it fetches a tuple in es_epqTuple[] array and
checks its visibility (ForeignRecheck() always say 'yep, it is visible'),
then ExecScan() applies its qualifiers by ExecQual().
So, as long as FDW/CSP can return a record that satisfies the TupleDesc
of this relation, made by the tuples in es_epqTuple[] array, rest of the
code paths are common.
I have an idea to solve the problem.
It adds recheckMtd() call if scanrelid==0 just before the assertion above,
and add a callback of FDW on ForeignRecheck().
The role of this new callback is to set up the supplied TupleTableSlot
and check its visibility, but does not define how to do this.
It is arbitrarily by FDW driver, like invocation of alternative plan
consists of only built-in logic.
Invocation of alternative plan is one of the most feasible way to
implement EPQ logic on FDW, so I think FDW also needs a mechanism
that takes child path-nodes like custom_paths in CustomPath node.
Once a valid path node is linked to this list, createplan.c transform
them to relevant plan node, then FDW can initialize and invoke this
plan node during execution, like ForeignRecheck().
This design can solve another problem Fujita-san has also mentioned.
If scan qualifier is pushed-down to the remote query and its expression
node is saved in the private area of ForeignScan, the callback on
ForeignRecheck() can evaluate the qualifier by itself. (Note that only
FDW driver can know where and how expression node being pushed-down
is saved in the private area.)
In the summary, the following three enhancements are a straightforward
way to fix up the problem he reported.
1. Add a special path to call recheckMtd in ExecScanFetch if scanrelid==0
2. Add a callback of FDW in ForeignRecheck() - to construct a record
according to the fdw_scan_tlist definition and to evaluate its
visibility, or to evaluate qualifier pushed-down if base relation.
3. Add List *fdw_paths in ForeignPath like custom_paths of CustomPaths,
to construct plan nodes for EPQ evaluation.
I'm not an expert in this area, but this plan does not seem unreasonable to
me.
IIRC the discussion with KaiGai-san, I think that that would work. I
think that that would be more suitable for CSPs, though. Correct me if
I'm wrong, KaiGai-san. In either case, I'm not sure that the idea of
transferring both processing to a single callback routine hooked in
ForeignRecheck is a good idea: (a) check to see if the test tuple for
each component foreign table satisfies the remote qual condition and (b)
check to see if those tuples satisfy the remote join condition. I think
that that would be too complicated, probably making the callback routine
bug-prone. So, I'd still propose that
Re: [HACKERS] Macro nesting hell
... btw, why don't we convert c.h's Max(), Min(), and Abs() to inlines? They've all got multi-eval hazards. It might also be interesting to research whether inline would allow simplifying the MemSetFoo family. 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] can't coax query planner into using all columns of a gist index
What's a good way for me to create a self-contained test case. AFAIU the only way to make these test cases more self-contained would be to inline the second table and its index. How do you create an index to an inlined table of values? Or perhaps I could send over a dump of a subset of the data? Yes, I am fairly sure that I am running 9.4.4: $ psql --version psql (PostgreSQL) 9.4.4 # select version(); version --- PostgreSQL 9.4.4 on x86_64-unknown-linux-gnu, compiled by gcc (GCC) 5.1.0, 64-bit (1 row) Thanks for the help, Gideon. On Tue, Aug 11, 2015 at 10:23 PM Tom Lane t...@sss.pgh.pa.us wrote: Gideon Dresdner gide...@gmail.com writes: I had a discussion on IRC today with RhodiumToad regarding optimizing a specific query. We didn't manage to figure out how to get postgres to hit a GIST index. FWIW, I couldn't reproduce the described behavior. Can you provide a self-contained test case? Are you sure your server is 9.4.4? regards, tom lane
Re: [HACKERS] fix oversight converting buf_id to Buffer
Hi, On 2015-08-11 01:15:37 +0200, Andres Freund wrote: I'm too tired right now to look at this, but it generally looked sane. Pushed your fix to master and 9.5, with two very minor changes: 1) I moved the BufferDescriptorGetBuffer() call in PinBuffer_Locked() to after the spinlock release. It's rather minor, but there seems little reason to do it before except the assert, which isn't compiled in production. 2) I removed the two asserts you added. They essentially asserted that i + 1 == i + 1. Thanks again for the catch and patch. 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] Test code is worth the space
* Robert Haas (robertmh...@gmail.com) wrote: On Wed, Aug 12, 2015 at 2:04 PM, Peter Geoghegan p...@heroku.com wrote: This resistance to adding tests seems quite short sighted to me, especially when the concern is about queries that will each typically take less than 1ms to execute. Like Noah, I think that it would be very helpful to simply be more inclusive of additional tests that don't increase test coverage by as much as each query in a minimal subset. I am not at all convinced by arguments about the cost of maintaining tests when a simple behavioral change occurs. I've removed tests from patches that in my opinion were unlikely to fail either (a) for any reason or (b) for any reason other than an intentional change, and I think that's a reasonable thing to do. However, I still think it's a good idea, and useful, to try to expand the code coverage we get from 'make check'. However, the bigger issue, IMHO, is the stuff that can't be tested via pg_regress, e.g. because it needs hooks, like what Alvaro is talking about, or because it needs a custom testing framework. Recovery, for example, really needs a lot more testing, as we talked about at PGCon. If we just expand what 'make check' covers and don't deal with those kinds of things, we will be improving our test coverage but maybe not all that much. Agreed, and this is something which I said we'd work to help with and which we have some things to show now, for those interested. The regression tests included in pgBackRest (available here: https://github.com/pgmasters/backrest) go through a number of different recovery tests. There's vagrant configs for a few different VMs too (CentOS 6, CentOS 7, Ubuntu 12.04 and Ubuntu 14.04) which is what we've been testing. We're working to continue expanding those tests and will also be adding tests for replication and promotion in the future. Eventually, we plan to write a buildfarm module for pgBackRest, to allow it to be run in the same manner as the regular buildfarm animals with the results posted. David, feel free to correct me if I'm misconstrued anything above. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] GinPageIs* don't actually return a boolean
On 2015-08-12 18:52:59 -0400, Tom Lane wrote: Andres Freund and...@anarazel.de writes: I went through all headers in src/include and checked for macros containing [^][^] and checked whether they have this hazard. Found a fair number. That patch also changes !! tests into != 0 style. Looks OK to me, except I wonder why you did this #define TRIGGER_FIRED_FOR_ROW(event) \ - ((event) TRIGGER_EVENT_ROW) + (((event) TRIGGER_EVENT_ROW) == TRIGGER_EVENT_ROW) rather than != 0. That way doesn't look either more efficient or more readable. Purely consistency with the surrounding code. I was on the fence about that one... -- 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] GinPageIs* don't actually return a boolean
Andres Freund and...@anarazel.de writes: On 2015-08-12 18:52:59 -0400, Tom Lane wrote: Looks OK to me, except I wonder why you did this #define TRIGGER_FIRED_FOR_ROW(event) \ -((event) TRIGGER_EVENT_ROW) +(((event) TRIGGER_EVENT_ROW) == TRIGGER_EVENT_ROW) rather than != 0. That way doesn't look either more efficient or more readable. Purely consistency with the surrounding code. I was on the fence about that one... The adjacent code is doing something different than a bit-test, though: it's checking whether multibit fields have particular values. 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] Raising our compiler requirements for 9.6
On Wed, Aug 12, 2015 at 4:34 PM, Heikki Linnakangas hlinn...@iki.fi wrote: Andres didn't mention how big the performance benefit he saw with pgbench was, but I bet it was barely distinguishible from noise. But that's OK. In fact, there's no reason to believe this would make any difference to performance. The point is to make the code more readable, and it certainly achieves that. I think that when Bruce macro-ized this ten years ago or whenever it was, he got a significant performance benefit from it; otherwise I don't think he would have done it. It may well be that the march of time has improved compiler technology to the point where no benefit remains, and that's fine. But just as with any other part of the code, we shouldn't start with the premise that the existing code is bad the way it is. If a careful analysis leads us to that conclusion, that's fine. In this particular case, I am more concerned about making sure that people who have an opinion get a chance to air it than I am with the outcome. I have no reason to believe that we shouldn't make this change; I merely want to make sure that anyone who does have a concern about this (or other changes in this area) has a chance to be heard before we get too far down the path. Nobody's going to want to turn back the clock once we do this. -- 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] count_nulls(VARIADIC any)
On Wed, Aug 12, 2015 at 4:11 PM, Tom Lane t...@sss.pgh.pa.us wrote: Peter Geoghegan p...@heroku.com writes: On Wed, Aug 12, 2015 at 10:30 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: The name count_nulls() suggest an aggregate function to me, though. I thought the same. Ditto. I'd be fine with this if we can come up with a name that doesn't sound like an aggregate. The best I can do offhand is number_of_nulls(), which doesn't seem very pretty. nulls_in(a, b, c) IN (0, 3) - yes the repetition is not great... David J.
Re: [HACKERS] Raising our compiler requirements for 9.6
On Wed, Aug 12, 2015 at 1:34 PM, Heikki Linnakangas hlinn...@iki.fi wrote: Andres didn't mention how big the performance benefit he saw with pgbench was, but I bet it was barely distinguishible from noise. But that's OK. In fact, there's no reason to believe this would make any difference to performance. The point is to make the code more readable, and it certainly achieves that. +1 -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Raising our compiler requirements for 9.6
On Wed, Aug 12, 2015 at 2:57 PM, Andres Freund and...@anarazel.de wrote: The only actual separate patch since then (fastgetattr as inline function) was posted 2015-08-05 and I yesterday suggested to push it today. And it's just replacing two existing macros by inline functions. I'm a little concerned about that one. Your testing shows that's a win for you, but is it a win for everyone? Maybe we should go ahead and do it anyway, but I'm not sure. -- 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] WIP: SCRAM authentication
Robert, * Robert Haas (robertmh...@gmail.com) wrote: On Wed, Aug 12, 2015 at 4:09 PM, Stephen Frost sfr...@snowman.net wrote: As for the notion of dropping md5 from 9.6 or even forcing it to be one-or-the-other on a per-role basis, ... Please don't conflate those two things. They are radically different in terms of the amount of upgrade pain that they cause. The first one would be completely insane. Thanks for the clarification. I had gotten the (apparently mistaken) impression[1] that there was some consideration for a hard break from one release to the next to move from md5 to SCRAM. Would be great to get comments on the other comments, specifically that adding SCRAM's password verifier won't seriously change the security of a user's account or password based on an attack vector where the contents of pg_authid is compromised. I do agree with the general concern that the additional complexity involved in supporting multiple password verifiers may result in bugs, and likely security ones, but I really expect the larger risk to be from the SCRAM implementation itself than how we get data into and back out of our own catalogs. Thanks! Stephen [1]: CA+TgmoYQ=8br87xggkews8hjse3kqh5v4fq+bz2sazhanh3...@mail.gmail.com signature.asc Description: Digital signature
Re: [HACKERS] Warnings around booleans
* Andres Freund (and...@anarazel.de) wrote:
I find that a somewhat ugly coding pattern, but since the rest of the
function is written that way...
Agreed, but not going to change it at this point.
Would love feedback on the attached. I included the variable renames
discussed previously with Noah as they're quite minor changes.
Had no trouble cherry-picking this back to 9.5.
I'll do that and add regression tests for it and any others which don't
get tested. My thinking on the test is to independently change each
value and then poll for all role attributes set and verify that the only
change made was the change expected.
Do that if you like, but what I really think we should have is a test
that tries to bypass rls and fails, then the user gets changes and it
succeeds, and then it gets disabled and fails again. This really seems
test-worthy behaviour to me.
I'll look at doing this also in the rowsecurity regression suite, but I
really like having this coverage of CREATE/ALTER ROLE too, plus testing
the role dump/restore paths in pg_dumpall which I don't think were being
covered at all previously...
Thanks!
Stephen
From ab44660e9b8fc5b10f84ce6ba99a8340047ac8a5 Mon Sep 17 00:00:00 2001
From: Stephen Frost sfr...@snowman.net
Date: Wed, 12 Aug 2015 15:50:54 -0400
Subject: [PATCH] In AlterRole, make bypassrls an int
When reworking bypassrls in AlterRole to operate the same way the other
attribute handling is done, I missed that the variable was incorrectly a
bool rather than an int. This meant that on platforms with an unsigned
char, we could end up with incorrect behavior during ALTER ROLE.
Pointed out by Andres thanks to tests he did changing our bool to be the
one from stdbool.h which showed this and a number of other issues.
Add regression tests to test CREATE/ALTER role for the various role
attributes. Arrange to leave roles behind for testing pg_dumpall, but
none which have the LOGIN attribute.
In passing, to avoid confusion, rename CreatePolicyStmt's 'cmd' to
'cmd_name', parse_policy_command's 'cmd' to 'polcmd', and
AlterPolicy's 'cmd_datum' to 'polcmd_datum', per discussion with Noah
and as a follow-up to his correction of copynodes/equalnodes handling of
the CreatePolicyStmt 'cmd' field.
Back-patch to 9.5 where the AlterRole bug exists and to avoid the code
diverging due to minor changes while in alpha.
---
src/backend/commands/policy.c| 22 +--
src/backend/commands/user.c | 2 +-
src/backend/nodes/copyfuncs.c| 2 +-
src/backend/nodes/equalfuncs.c | 2 +-
src/backend/parser/gram.y| 2 +-
src/include/nodes/parsenodes.h | 2 +-
src/test/regress/expected/roleattributes.out | 236 +++
src/test/regress/parallel_schedule | 2 +-
src/test/regress/serial_schedule | 1 +
src/test/regress/sql/roleattributes.sql | 85 ++
10 files changed, 339 insertions(+), 17 deletions(-)
create mode 100644 src/test/regress/expected/roleattributes.out
create mode 100644 src/test/regress/sql/roleattributes.sql
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index bcf4a8f..45326a3 100644
--- a/src/backend/commands/policy.c
+++ b/src/backend/commands/policy.c
@@ -108,25 +108,25 @@ RangeVarCallbackForPolicy(const RangeVar *rv, Oid relid, Oid oldrelid,
static char
parse_policy_command(const char *cmd_name)
{
- char cmd;
+ char polcmd;
if (!cmd_name)
elog(ERROR, unrecognized policy command);
if (strcmp(cmd_name, all) == 0)
- cmd = '*';
+ polcmd = '*';
else if (strcmp(cmd_name, select) == 0)
- cmd = ACL_SELECT_CHR;
+ polcmd = ACL_SELECT_CHR;
else if (strcmp(cmd_name, insert) == 0)
- cmd = ACL_INSERT_CHR;
+ polcmd = ACL_INSERT_CHR;
else if (strcmp(cmd_name, update) == 0)
- cmd = ACL_UPDATE_CHR;
+ polcmd = ACL_UPDATE_CHR;
else if (strcmp(cmd_name, delete) == 0)
- cmd = ACL_DELETE_CHR;
+ polcmd = ACL_DELETE_CHR;
else
elog(ERROR, unrecognized policy command);
- return cmd;
+ return polcmd;
}
/*
@@ -480,7 +480,7 @@ CreatePolicy(CreatePolicyStmt *stmt)
int i;
/* Parse command */
- polcmd = parse_policy_command(stmt-cmd);
+ polcmd = parse_policy_command(stmt-cmd_name);
/*
* If the command is SELECT or DELETE then WITH CHECK should be NULL.
@@ -674,7 +674,7 @@ AlterPolicy(AlterPolicyStmt *stmt)
bool replaces[Natts_pg_policy];
ObjectAddress target;
ObjectAddress myself;
- Datum cmd_datum;
+ Datum polcmd_datum;
char polcmd;
bool polcmd_isnull;
int i;
@@ -775,11 +775,11 @@ AlterPolicy(AlterPolicyStmt *stmt)
RelationGetRelationName(target_table;
/* Get policy command */
- cmd_datum = heap_getattr(policy_tuple, Anum_pg_policy_polcmd,
+ polcmd_datum = heap_getattr(policy_tuple, Anum_pg_policy_polcmd,
RelationGetDescr(pg_policy_rel),
polcmd_isnull);
Assert(!polcmd_isnull);
- polcmd =
Re: [HACKERS] GinPageIs* don't actually return a boolean
On 2015-08-12 19:03:50 -0400, Tom Lane wrote: The adjacent code is doing something different than a bit-test, though: it's checking whether multibit fields have particular values. Yea, I know, that's why I was on the fence about it. Since you have an opinion and I couldn't really decide it's pretty clear which direction to go ;) -- 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] Test code is worth the space
On Wed, Aug 12, 2015 at 7:20 PM, Tom Lane t...@sss.pgh.pa.us wrote: FWIW, I've objected in the past to tests that would significantly increase the runtime of make check, unless I thought they were especially valuable (which enumerating every minor behavior of a feature patch generally isn't IMO). I still think that that's an important consideration: every second you add to make check is multiplied many times over when you consider how many developers run that how many times a day. We've talked about having some sort of second rank of tests that people wouldn't necessarily run before committing, and that would be allowed to eat more time than the core regression tests would. I think that might be a valuable direction to pursue if people start submitting very bulky tests. Maybe. Adding a whole new test suite is significantly more administratively complex, because the BF client has to get updated to run it. And if expected outputs in that test suite change very often at all, then committers will have to run it before committing anyway. The value of a core regression suite that takes less time to run has to be weighed against the possibility that a better core regression suite might cause us to find more bugs before committing. That could easily be worth the price in runtime. -- 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] [COMMITTERS] pgsql: Close some holes in BRIN page assignment
On 2015-08-12 16:08:08 -0300, Alvaro Herrera wrote: Alvaro Herrera wrote: Close some holes in BRIN page assignment buildfarm evidently didn't like this one :-( clang seems to see a (the?) problem: /home/andres/src/postgresql/src/backend/access/brin/brin_pageops.c:357:6: warning: variable 'extended' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized] if (!BufferIsValid(*buffer)) ^~~ /home/andres/src/postgresql/src/backend/access/brin/brin_pageops.c:371:6: note: uninitialized use occurs here if (extended) ^~~~ /home/andres/src/postgresql/src/backend/access/brin/brin_pageops.c:357:2: note: remove the 'if' if its condition is always true if (!BufferIsValid(*buffer)) ^~~~ /home/andres/src/postgresql/src/backend/access/brin/brin_pageops.c:330:16: note: initialize the variable 'extended' to silence this warning boolextended; Looks to me like it's right. That also explains why it's failing on a fairly random selection of platforms and compilers... 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] Macro nesting hell
On 2015-08-12 10:18:12 -0400, Tom Lane wrote:
Andres Freund and...@anarazel.de writes:
On 2015-07-01 12:55:48 -0300, Alvaro Herrera wrote:
Tom Lane wrote:
I'm thinking we really ought to mount a campaign to replace some of these
macros with inlined-if-possible functions.
My guess is that changing a very small amount of them will do a large
enough portion of the job.
I think it'd be good to convert the macros in bufpage.h and bufmgr.h
that either currently have multiple evaluation hazards, or have a
AssertMacro() in them. The former for obvious reasons, the latter
because that immediately makes them rather large (on and it implies
multiple evaluation hazards anyway).
That'd mean inlining PageSetPageSizeAndVersion(), PageGetSpecialSize(),
PageGetSpecialPointer(), PageGetItem(), PageGetMaxOffsetNumber(),
PageIsPrunable(), PageSetPrunable(), PageClearPrunable(),
BufferIsValid(), BufferGetBlock(), BufferGetPageSize().
Sounds reasonable to me. If you do this, I'll see whether pademelon
can be adjusted to build using the minimum macro expansion buffer
size specified by the C standard.
Here's the patch attached.
There's two more macros on the list that I had missed:
PageXLogRecPtrSet(), PageXLogRecPtrGet(). Unfortunately *Set() requires
to pas a pointer to the PageXLogRecPtr - there's only two callers tho.
We could instead just leave these, or add an indirecting macro. Seems
fine to me though.
With it applied pg compiles without the warning I saw before:
/home/andres/src/postgresql/src/backend/access/brin/brin_pageops.c:778:5:
warning: string literal of length 7732 exceeds maximum length 4095
that ISO C99 compilers are required to support [-Woverlength-strings]
Assert(BRIN_IS_REGULAR_PAGE(BufferGetPage(oldbuf)));
^~
We could obviously be more aggressive and convert all the applicable
defines, but they are already readable and have no multiple eval
hazards, so I'm not inclined to that.
Andres
From 8203fd286c276051060a6fe8c98c8b576c644f14 Mon Sep 17 00:00:00 2001
From: Andres Freund and...@anarazel.de
Date: Thu, 13 Aug 2015 01:37:44 +0200
Subject: [PATCH] Change some buffer and page related macros to inline
functions.
Firstly some of these macros could recursively expand to string literals
larger than the minimum required to be supported by an environment by
the C standard. That's primarily due to included assertions. Secondly
some macros had multiple evaluation hazards.
This required one minor API change in that PageXLogRecPtrSet now takes a
pointer to a PageXLogRecPtr instead the value directly. There shouldn't
be callers to it out there...
Discussion: 4407.1435763...@sss.pgh.pa.us
---
src/include/access/gist.h | 2 +-
src/include/storage/bufmgr.h | 29 +-
src/include/storage/bufpage.h | 124 ++
3 files changed, 95 insertions(+), 60 deletions(-)
diff --git a/src/include/access/gist.h b/src/include/access/gist.h
index 81e559b..33eb9d3 100644
--- a/src/include/access/gist.h
+++ b/src/include/access/gist.h
@@ -142,7 +142,7 @@ typedef struct GISTENTRY
#define GistClearFollowRight(page) ( GistPageGetOpaque(page)-flags = ~F_FOLLOW_RIGHT)
#define GistPageGetNSN(page) ( PageXLogRecPtrGet(GistPageGetOpaque(page)-nsn))
-#define GistPageSetNSN(page, val) ( PageXLogRecPtrSet(GistPageGetOpaque(page)-nsn, val))
+#define GistPageSetNSN(page, val) ( PageXLogRecPtrSet(GistPageGetOpaque(page)-nsn, val))
/*
* Vector of GISTENTRY structs; user-defined methods union and picksplit
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index ec0a254..235264c 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -96,11 +96,12 @@ extern PGDLLIMPORT int32 *LocalRefCount;
* even in non-assert-enabled builds can be significant. Thus, we've
* now demoted the range checks to assertions within the macro itself.
*/
-#define BufferIsValid(bufnum) \
-( \
- AssertMacro((bufnum) = NBuffers (bufnum) = -NLocBuffer), \
- (bufnum) != InvalidBuffer \
-)
+static inline bool
+BufferIsValid(Buffer bufnum)
+{
+ AssertArg(bufnum = NBuffers bufnum = -NLocBuffer);
+ return bufnum != InvalidBuffer;
+}
/*
* BufferGetBlock
@@ -109,14 +110,16 @@ extern PGDLLIMPORT int32 *LocalRefCount;
* Note:
* Assumes buffer is valid.
*/
-#define BufferGetBlock(buffer) \
-( \
- AssertMacro(BufferIsValid(buffer)), \
- BufferIsLocal(buffer) ? \
- LocalBufferBlockPointers[-(buffer) - 1] \
- : \
- (Block) (BufferBlocks + ((Size) ((buffer) - 1)) * BLCKSZ) \
-)
+static inline Block
+BufferGetBlock(Buffer buffer)
+{
+ AssertArg(BufferIsValid(buffer));
+
+ if (BufferIsLocal(buffer))
+ return LocalBufferBlockPointers[-buffer - 1];
+ else
+ return (Block) (BufferBlocks + ((Size) (buffer - 1)) * BLCKSZ);
+}
/*
* BufferGetPageSize
diff --git a/src/include/storage/bufpage.h
Re: [HACKERS] WIP: SCRAM authentication
On Wed, Aug 12, 2015 at 4:09 PM, Stephen Frost sfr...@snowman.net wrote: As for the notion of dropping md5 from 9.6 or even forcing it to be one-or-the-other on a per-role basis, ... Please don't conflate those two things. They are radically different in terms of the amount of upgrade pain that they cause. The first one would be completely insane. -- 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] WIP: SCRAM authentication
On Wed, Aug 12, 2015 at 4:37 PM, Stephen Frost sfr...@snowman.net wrote: Please don't conflate those two things. They are radically different in terms of the amount of upgrade pain that they cause. The first one would be completely insane. Thanks for the clarification. I had gotten the (apparently mistaken) impression[1] that there was some consideration for a hard break from one release to the next to move from md5 to SCRAM. Gosh, no way. Sorry if I gave the contrary impression; that was certainly not my intention. As I said before, I have no reason to believe that MD5 is insecure in the way we are using it. Unless much better attacks are published, I don't really care if people are still using it 5 years from now, or even 10. I just want to give people an *option* to move away from it, because: (1) It's possible that better attacks may be published, and (2) Even if they aren't, some people don't want to use anything that says MD5 on the tin. Would be great to get comments on the other comments, specifically that adding SCRAM's password verifier won't seriously change the security of a user's account or password based on an attack vector where the contents of pg_authid is compromised. I do agree with the general concern that the additional complexity involved in supporting multiple password verifiers may result in bugs, and likely security ones, but I really expect the larger risk to be from the SCRAM implementation itself than how we get data into and back out of our own catalogs. Yes, the SCRAM implementation could be buggy. But also, OpenSSL has repeatedly had security bugs that were due to forcing servers to downgrade to older protocols. I wouldn't like us to start growing similar vulnerabilities, where SCRAM would have been just fine but an attack that involves forcing a downgrade to MD5 is possible. I don't think you are quite correct about the scenario where pg_authid is compromised. Even if the hash stored there is functionally equivalent to the password itself as far as this instance of PostgreSQL is concerned, the same password may have been used for other services, so cracking it has a purpose. -- 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] GinPageIs* don't actually return a boolean
Andres Freund and...@anarazel.de writes: I went through all headers in src/include and checked for macros containing [^][^] and checked whether they have this hazard. Found a fair number. That patch also changes !! tests into != 0 style. Looks OK to me, except I wonder why you did this #define TRIGGER_FIRED_FOR_ROW(event) \ - ((event) TRIGGER_EVENT_ROW) + (((event) TRIGGER_EVENT_ROW) == TRIGGER_EVENT_ROW) rather than != 0. That way doesn't look either more efficient or more readable. 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] Test code is worth the space
Robert Haas robertmh...@gmail.com writes: On Wed, Aug 12, 2015 at 2:04 PM, Peter Geoghegan p...@heroku.com wrote: This resistance to adding tests seems quite short sighted to me, especially when the concern is about queries that will each typically take less than 1ms to execute. Like Noah, I think that it would be very helpful to simply be more inclusive of additional tests that don't increase test coverage by as much as each query in a minimal subset. I am not at all convinced by arguments about the cost of maintaining tests when a simple behavioral change occurs. I've removed tests from patches that in my opinion were unlikely to fail either (a) for any reason or (b) for any reason other than an intentional change, and I think that's a reasonable thing to do. FWIW, I've objected in the past to tests that would significantly increase the runtime of make check, unless I thought they were especially valuable (which enumerating every minor behavior of a feature patch generally isn't IMO). I still think that that's an important consideration: every second you add to make check is multiplied many times over when you consider how many developers run that how many times a day. We've talked about having some sort of second rank of tests that people wouldn't necessarily run before committing, and that would be allowed to eat more time than the core regression tests would. I think that might be a valuable direction to pursue if people start submitting very bulky tests. 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] checkpointer continuous flushing
Here is a v8, I collected a few performance figures with this patch on an old box with 8 cores, 16 GB, RAID 1 HDD, under Ubuntu precise. postgresql.conf: shared_buffers = 4GB checkpoint_timeout = 15min checkpoint_completion_target = 0.8 max_wal_size = 4GB init pgbench -i -s 250 warmup pgbench -T 1200 -M prepared -S -j 2 -c 4 # 400 tps throttled simple update test sh pgbench -M prepared -N -P 1 -T 4000 -R 400 -L 100 -j 2 -c 4 sort/flush : percent of skipped/late transactions on on : 2.7 on off : 16.2 off on : 68.4 off off : 68.7 # 200 tps sh pgbench -M prepared -N -P 1 -T 4000 -R 200 -L 100 -j 2 -c 4 sort/flush : percent of skipped/late transactions on on : 2.7 on off : 9.5 off on : 47.4 off off : 48.8 The large percent of skipped/late transactions is to be understood as fraction of time with postgresql offline because of a write stall. # full speed 1 client sh pgbench -M prepared -N -P 1 -T 4000 sort/flush : tps avg stddev (percent of time beyond 10.0 tps) on on : 631 +- 131 (0.1%) on off : 564 +- 303 (12.0%) off on : 167 +- 315 (76.8%) # stuck... off off : 177 +- 305 (71.2%) # ~ current pg # full speed 2 threads 4 clients sh pgbench -M prepared -N -P 1 -T 4000 -j 2 -c 4 sort/flush : tps avg stddev (percent of time below 10.0 tps) on on : 1058 +- 455 (0.1%) on off : 1056 +- 942 (32.8%) off on : 170 +- 500 (88.3%) # stuck... off off : 209 +- 506 (82.0%) # ~ current pg The combined features provide a tps speedup of 3-5 on these runs, and allow to have some control on write stalls. Flushing is not effective on unsorted buffers, at least on these example. -- 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] Raising our compiler requirements for 9.6
On 08/12/2015 11:25 PM, Robert Haas wrote: On Wed, Aug 12, 2015 at 2:57 PM, Andres Freund and...@anarazel.de wrote: The only actual separate patch since then (fastgetattr as inline function) was posted 2015-08-05 and I yesterday suggested to push it today. And it's just replacing two existing macros by inline functions. I'm a little concerned about that one. Your testing shows that's a win for you, but is it a win for everyone? Maybe we should go ahead and do it anyway, but I'm not sure. Andres didn't mention how big the performance benefit he saw with pgbench was, but I bet it was barely distinguishible from noise. But that's OK. In fact, there's no reason to believe this would make any difference to performance. The point is to make the code more readable, and it certainly achieves that. - Heikki -- 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] can't coax query planner into using all columns of a gist index
Gideon Dresdner gide...@gmail.com writes: I've created a small dump of my database that recreates the problem. I hope that this will help recreate the problem. It is attached. I'd be happy to hear if there is an easier way of doing this. Ah. Now that I see the database schema, the problem is here: regression=# \d vcf ... chr | smallint | ... So chr is smallint in one table and integer in the other. That means the parser translates qcregions.chr = vcf.chr using the int42eq operator instead of int4eq --- and nobody's ever taught btree_gist about crosstype operators. So the clause simply isn't considered indexable with this index. If you change the query to qcregions.chr = vcf.chr::int then all is well. Personally I'd just change vcf.chr to integer --- it's not even saving you any space, with that table schema, because the next column has to be int-aligned anyway. 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] count_nulls(VARIADIC any)
Peter Geoghegan p...@heroku.com writes: On Wed, Aug 12, 2015 at 10:30 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: The name count_nulls() suggest an aggregate function to me, though. I thought the same. Ditto. I'd be fine with this if we can come up with a name that doesn't sound like an aggregate. The best I can do offhand is number_of_nulls(), which doesn't seem very pretty. 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] Raising our compiler requirements for 9.6
On 2015-08-12 16:25:17 -0400, Robert Haas wrote: On Wed, Aug 12, 2015 at 2:57 PM, Andres Freund and...@anarazel.de wrote: The only actual separate patch since then (fastgetattr as inline function) was posted 2015-08-05 and I yesterday suggested to push it today. And it's just replacing two existing macros by inline functions. I'm a little concerned about that one. Your testing shows that's a win for you, but is it a win for everyone? Maybe we should go ahead and do it anyway, but I'm not sure. I don't think it's likely to affect performance in any significant way in either direction. I mean, the absolute worst case would be that a formerly in-place fastgetattr() call gets changed into a function call in the same translation unit. That might or might not have a performance impact in either direction, but it's not going to be large. The only reason this has improved performance is imo that it shrank the code size a bit (increasing cache hit ratio, increase use of the branch prediction unit etc.). In all the profiles I've seen where fastgetattr (or rather the in-place code it has) is responsible for some portion of the runtime it was the actual looping, the cache misses, et al, not the stack and the indirect call. It's debatable that it's inline (via macro or inline function) in the first place. The advantage is not performance, it's readability. I've several times re-wrapped fastgetattr() just to understand what it does, because I otherwise find the code so hard to read. Maybe I just utterly suck at reading macros... You might argue that it's nothing we have touched frequently. And you're right. But I think that's a mistake. We spend far too much time in the various pieces of code dissembling tuples, and I think at some point somebody really needs to spend time on this. Regards, Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Test code is worth the space
On Wed, Aug 12, 2015 at 2:04 PM, Peter Geoghegan p...@heroku.com wrote: This resistance to adding tests seems quite short sighted to me, especially when the concern is about queries that will each typically take less than 1ms to execute. Like Noah, I think that it would be very helpful to simply be more inclusive of additional tests that don't increase test coverage by as much as each query in a minimal subset. I am not at all convinced by arguments about the cost of maintaining tests when a simple behavioral change occurs. I've removed tests from patches that in my opinion were unlikely to fail either (a) for any reason or (b) for any reason other than an intentional change, and I think that's a reasonable thing to do. However, I still think it's a good idea, and useful, to try to expand the code coverage we get from 'make check'. However, the bigger issue, IMHO, is the stuff that can't be tested via pg_regress, e.g. because it needs hooks, like what Alvaro is talking about, or because it needs a custom testing framework. Recovery, for example, really needs a lot more testing, as we talked about at PGCon. If we just expand what 'make check' covers and don't deal with those kinds of things, we will be improving our test coverage but maybe not all that much. -- 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] WIP: SCRAM authentication
On 08/12/2015 01:37 PM, Stephen Frost wrote: Would be great to get comments on the other comments, specifically that adding SCRAM's password verifier won't seriously change the security of a user's account or password based on an attack vector where the contents of pg_authid is compromised. I do agree with the general concern that the additional complexity involved in supporting multiple password verifiers may result in bugs, and likely security ones, but I really expect the larger risk to be from the SCRAM implementation itself than how we get data into and back out of our own catalogs. There's also the concern that the additional complexity will cause *users* to make security-compromising mistakes, which I think is the greater risk. Robert has mostly won me over to his point of view on this. The only case where I can see multiple verifiers per role making a real difference in migrations is for PGAAS hosting. But the folks from Heroku and AWS have been notably silent on this; lemme ping them. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] GIN pending list clean up exposure to SQL
I've written a function which allows users to clean up the pending list.
It takes the index name and returns the number of pending list pages
deleted.
# select * from gin_clean_pending_list('foo_text_array_idx');
gin_clean_pending_list
278
(1 row)
Time: 31994.880 ms
This is needed because there needs to be a way to offload this duty from
the user backends, and the only other way to intentionaly clean up the list
is by vacuum (and the rest of a vacuum can take days to run on a large
table). Autoanalyze will also do it, but it hard to arrange for those to
occur at need, and unless you drop default_statistics_target very low they
can also take a long time. And if you do lower the target, it screws up
your statistics, of course.
I've currently crammed it into pageinspect, simply because that is where I
found the existing code which I used as an exemplar for writing this code.
But where does this belong? Core? Its own separate extension?
Cheers,
Jeff
gin_clean_pending_user_v001.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] Raising our compiler requirements for 9.6
On 2015-08-12 23:34:38 +0300, Heikki Linnakangas wrote: Andres didn't mention how big the performance benefit he saw with pgbench was, but I bet it was barely distinguishible from noise. I think it was discernible (I played around with changing unrelated code trying to exclude unrelated layout issues), but it definitely was small. I think the only reason it had any effect is the reduced overall code size. The point is to make the code more readable, and it certainly achieves that. Exactly. - 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] WIP: SCRAM authentication
* Josh Berkus (j...@agliodbs.com) wrote: On 08/12/2015 01:37 PM, Stephen Frost wrote: Would be great to get comments on the other comments, specifically that adding SCRAM's password verifier won't seriously change the security of a user's account or password based on an attack vector where the contents of pg_authid is compromised. I do agree with the general concern that the additional complexity involved in supporting multiple password verifiers may result in bugs, and likely security ones, but I really expect the larger risk to be from the SCRAM implementation itself than how we get data into and back out of our own catalogs. There's also the concern that the additional complexity will cause *users* to make security-compromising mistakes, which I think is the greater risk. Robert has mostly won me over to his point of view on this. That is certainly an issue to address- but that's one which I believe we can address a great deal better than what we're doing with the currently proposed patch. I do feel we need to provide flexibility and options, but we also need to consider the simple case and make sure that it remains simple. The only case where I can see multiple verifiers per role making a real difference in migrations is for PGAAS hosting. But the folks from Heroku and AWS have been notably silent on this; lemme ping them. While their insight is certainly valuable, they are certainly not the only cases of one-user-to-rule-them-all environments. Further, there's going to be cases where multiple applications from different languages are accessing the database through the same account because there's only one account. I'd rather not punt on those cases and, further, assume that we'll always be able to keep it to only one password verifier per account. As I tried to outline up-thread, there are a set of features which would be very nice for us to have which require further information to be saved beyond even these different password verifiers for each. As mentioned elsewhere, even SCRAM is possible of having multiple password verifiers based on the various algorithms used. In other words, I doubt the 'only one password verifier per role' approach is going to work out for us long term in any case. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Test code is worth the space
On Thu, Aug 13, 2015 at 5:54 AM, Stephen Frost wrote: The regression tests included in pgBackRest (available here: https://github.com/pgmasters/backrest) go through a number of different recovery tests. There's vagrant configs for a few different VMs too (CentOS 6, CentOS 7, Ubuntu 12.04 and Ubuntu 14.04) which is what we've been testing. We're working to continue expanding those tests and will also be adding tests for replication and promotion in the future. Eventually, we plan to write a buildfarm module for pgBackRest, to allow it to be run in the same manner as the regular buildfarm animals with the results posted. Interesting. Do you mind if I pick up from it some ideas for the in-core replication test suite based on TAP stuff? That's still in the works for the 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] Parsing tuple contents
Vignesh Raghunathan vignesh.pg...@gmail.com writes: I am working on a project which requires going through each field inside a tuple without using postgresql. I have managed to iterate through each tuple inside a table by recycling postgres's code. However, for the part of parsing through each field in the tuple, I am not able to think of anything other than using a bunch of if/else or switch case statements to handle each postgresql datatype. I looked through postgresql's code base but I am unable to identify the part of code that might do this. Could anyone please let me know where to look? Well, as far as identifying the field boundaries is concerned, there are not that many cases: you basically only need to worry about typlen and typalign. heap_deform_tuple() would be a good model. Of course, if you want to print the values in some human-readable form, there is not going to be a good substitute for per-datatype code :-( 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] WIP: SCRAM authentication
On Thu, Aug 13, 2015 at 10:22 AM, Stephen Frost wrote: The only case where I can see multiple verifiers per role making a real difference in migrations is for PGAAS hosting. But the folks from Heroku and AWS have been notably silent on this; lemme ping them. While their insight is certainly valuable, they are certainly not the only cases of one-user-to-rule-them-all environments. Further, there's going to be cases where multiple applications from different languages are accessing the database through the same account because there's only one account. I'd rather not punt on those cases and, further, assume that we'll always be able to keep it to only one password verifier per account. As I tried to outline up-thread, there are a set of features which would be very nice for us to have which require further information to be saved beyond even these different password verifiers for each. While looking at this stuff, I have been wondering as well about moving the validutil field into the verifier catalog as well for example. That's one. As mentioned elsewhere, even SCRAM is possible of having multiple password verifiers based on the various algorithms used. In other words, I doubt the 'only one password verifier per role' approach is going to work out for us long term in any case. SCRAM is a definition for an authorization protocol which includes many verifiers, and the minimal requirement to consider that SCRAM is implemented in a system is to have SCRAM-SHA1, per here: http://tools.ietf.org/html/rfc5802 For example we may want to have in parallel one verifier for SCRAM-SHA1 and one for SCRAM-SHA256 for the same user, and I think that we cannot close the door either to other SASL protocols, which is why it may make sense to split the SCRAM patch into two with the basic message protocol infrastructure in place. -- 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] WIP: SCRAM authentication
Robert, * Robert Haas (robertmh...@gmail.com) wrote: On Wed, Aug 12, 2015 at 4:37 PM, Stephen Frost sfr...@snowman.net wrote: Please don't conflate those two things. They are radically different in terms of the amount of upgrade pain that they cause. The first one would be completely insane. Thanks for the clarification. I had gotten the (apparently mistaken) impression[1] that there was some consideration for a hard break from one release to the next to move from md5 to SCRAM. Gosh, no way. Sorry if I gave the contrary impression; that was certainly not my intention. As I said before, I have no reason to believe that MD5 is insecure in the way we are using it. Unless much better attacks are published, I don't really care if people are still using it 5 years from now, or even 10. I just want to give people an *option* to move away from it, because: No worries, thanks again for the clarification and my apologies for misunderstanding and jumping to an incorrect assumption. As for the question about MD5, I do feel that we're using it in about the best way possible, but I don't feel it's a terribly secure algorithm today. I'd really like to see people moving away from it sooner rather than later and that our defaults move to much more secure solutions. (1) It's possible that better attacks may be published, and (2) Even if they aren't, some people don't want to use anything that says MD5 on the tin. Would be great to get comments on the other comments, specifically that adding SCRAM's password verifier won't seriously change the security of a user's account or password based on an attack vector where the contents of pg_authid is compromised. I do agree with the general concern that the additional complexity involved in supporting multiple password verifiers may result in bugs, and likely security ones, but I really expect the larger risk to be from the SCRAM implementation itself than how we get data into and back out of our own catalogs. Yes, the SCRAM implementation could be buggy. But also, OpenSSL has repeatedly had security bugs that were due to forcing servers to downgrade to older protocols. I wouldn't like us to start growing similar vulnerabilities, where SCRAM would have been just fine but an attack that involves forcing a downgrade to MD5 is possible. I agree that such similar vulnerabilities would be unfortunate, but the way to avoid that is to not implement the actual hashing or encryption algorithms ourselves and to stick to the protocol as defined in the specification. I don't think you are quite correct about the scenario where pg_authid is compromised. Even if the hash stored there is functionally equivalent to the password itself as far as this instance of PostgreSQL is concerned, the same password may have been used for other services, so cracking it has a purpose. I attempted to address that also by stating that, should an attacker compromise a system with the goal of gaining the cleartext password, they would attempt the following, in order: 1) attempt to compromise a superuser account, if not already done, and then modify the system to get the 'password' auth mechanism to be used whereby the password is sent in the clear 2) change the existing password, or encourge the user to do so and somehow capture that activity 3) social engineering attacks 4) attempt to crack the md5 hash 5) attempt to crack the SCRAM password verifier 6) try to work out a way to use both the md5 hash and the SCRAM password verifier to figure out the password In terms of difficulty, while I believe the above is the right overall ordering, the level of difficulty goes up seriously between 4 and 5, with 6 requiring a level of expertise in hash algorithms and SCRAM that I have a very hard time seeing anyone bothering to go there, rather than just throw more CPUs at the md5 hash. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] GIN pending list clean up exposure to SQL
On Thu, Aug 13, 2015 at 2:19 AM, Jeff Janes jeff.ja...@gmail.com wrote:
I've written a function which allows users to clean up the pending list.
It takes the index name and returns the number of pending list pages
deleted.
# select * from gin_clean_pending_list('foo_text_array_idx');
gin_clean_pending_list
278
(1 row)
Time: 31994.880 ms
This is needed because there needs to be a way to offload this duty from
the user backends, and the only other way to intentionaly clean up the list
is by vacuum (and the rest of a vacuum can take days to run on a large
table). Autoanalyze will also do it, but it hard to arrange for those to
occur at need, and unless you drop default_statistics_target very low they
can also take a long time. And if you do lower the target, it screws up
your statistics, of course.
I've currently crammed it into pageinspect, simply because that is where I
found the existing code which I used as an exemplar for writing this code.
But where does this belong? Core? Its own separate extension?
For a long time we have gevel extension (
http://www.sigaev.ru/git/gitweb.cgi?p=gevel.git;a=summary) , which was
originally developed to help us debugging indexes, but it's also contains
user's functions. Your function could be there, but I have no idea about
gevel itself. Probably, it should be restructurized and come to contrib. Do
you have time to look into ?
Cheers,
Jeff
--
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] Test code is worth the space
On Wed, Aug 12, 2015 at 9:24 PM, Stephen Frost sfr...@snowman.net wrote: * Michael Paquier (michael.paqu...@gmail.com) wrote: On Thu, Aug 13, 2015 at 5:54 AM, Stephen Frost wrote: The regression tests included in pgBackRest (available here: https://github.com/pgmasters/backrest) go through a number of different recovery tests. There's vagrant configs for a few different VMs too (CentOS 6, CentOS 7, Ubuntu 12.04 and Ubuntu 14.04) which is what we've been testing. We're working to continue expanding those tests and will also be adding tests for replication and promotion in the future. Eventually, we plan to write a buildfarm module for pgBackRest, to allow it to be run in the same manner as the regular buildfarm animals with the results posted. Interesting. Do you mind if I pick up from it some ideas for the in-core replication test suite based on TAP stuff? That's still in the works for the next CF. Certainly don't mind at all, entirely open source under the MIT license. Why not the PG license? It would be nicer if we didn't have to worry about license contamination here. -- 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] [COMMITTERS] pgsql: Close some holes in BRIN page assignment
Andres Freund wrote: On 2015-08-12 16:08:08 -0300, Alvaro Herrera wrote: Alvaro Herrera wrote: Close some holes in BRIN page assignment buildfarm evidently didn't like this one :-( clang seems to see a (the?) problem: Ahh, right. There's an identical problem in the other function (brin_doupdate); maybe the conditional there is more complicated than it wants to analyze. I was trying randomized memory and wasn't finding anything ... BTW I looked around the buildfarm and couldn't find a single member that displayed these warnings :-( Looks to me like it's right. That also explains why it's failing on a fairly random selection of platforms and compilers... Yeah ... -- Álvaro Herrerahttp://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] Foreign join pushdown vs EvalPlanQual
Fujita-san,
The attached patch enhanced the FDW interface according to the direction
below (but not tested yet).
In the summary, the following three enhancements are a straightforward
way to fix up the problem he reported.
1. Add a special path to call recheckMtd in ExecScanFetch if scanrelid==0
2. Add a callback of FDW in ForeignRecheck() - to construct a record
according to the fdw_scan_tlist definition and to evaluate its
visibility, or to evaluate qualifier pushed-down if base relation.
3. Add List *fdw_paths in ForeignPath like custom_paths of CustomPaths,
to construct plan nodes for EPQ evaluation.
Likely, what you need to do are...
1. Save the alternative path on fdw_paths when foreign join push-down.
GetForeignJoinPaths() may be called multiple times towards a particular
joinrel according to the combination of innerrel/outerrel.
RelOptInfo-fdw_private allows to avoid construction of same remote
join path multiple times. On the second or later invocation, it may be
a good tactics to reference cheapest_startup_path and replace the saved
one if later invocation have cheaper one, prior to exit.
2. Save the alternative Plan nodes on fdw_plans or lefttree/righttree
somewhere you like at the GetForeignPlan()
3. Makes BeginForeignScan() to call ExecInitNode() towards the plan node
saved at (2), then save the PlanState on fdw_ps, lefttree/righttree,
or somewhere private area if not displayed on EXPLAIN.
4. Implement ForeignRecheck() routine. If scanrelid==0, it kicks the
planstate node saved at (3) to generate tuple slot. Then, call the
ExecQual() to check qualifiers being pushed down.
5. Makes EndForeignScab() to call ExecEndNode() towards the PlanState
saved at (3).
I never think above steps are too complicated for people who can write
FDW drivers. It is what developer usually does.
Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei kai...@ak.jp.nec.com
-Original Message-
From: Kaigai Kouhei(海外 浩平)
Sent: Wednesday, August 12, 2015 11:17 PM
To: 'Etsuro Fujita'; Robert Haas
Cc: PostgreSQL-development; 花田茂
Subject: RE: [HACKERS] Foreign join pushdown vs EvalPlanQual
-Original Message-
From: pgsql-hackers-ow...@postgresql.org
[mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Etsuro Fujita
Sent: Wednesday, August 12, 2015 8:26 PM
To: Robert Haas; Kaigai Kouhei(海外 浩平)
Cc: PostgreSQL-development; 花田茂
Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
On 2015/08/12 7:21, Robert Haas wrote:
On Fri, Aug 7, 2015 at 3:37 AM, Kouhei Kaigai kai...@ak.jp.nec.com
wrote:
I could have a discussion with Fujita-san about this topic.
Also, let me share with the discussion towards entire solution.
The primitive reason of this problem is, Scan node with scanrelid==0
represents a relation join that can involve multiple relations, thus,
its TupleDesc of the records will not fit base relations, however,
ExecScanFetch() was not updated when scanrelid==0 gets supported.
FDW/CSP on behalf of the Scan node with scanrelid==0 are responsible
to generate records according to the fdw_/custom_scan_tlist that
reflects the definition of relation join, and only FDW/CSP know how
to combine these base relations.
In addition, host-side expressions (like Plan-qual) are initialized
to reference the records generated by FDW/CSP, so the least invasive
approach is to allow FDW/CSP to have own logic to recheck, I think.
Below is the structure of ExecScanFetch().
ExecScanFetch(ScanState *node,
ExecScanAccessMtd accessMtd,
ExecScanRecheckMtd recheckMtd)
{
EState *estate = node-ps.state;
if (estate-es_epqTuple != NULL)
{
/*
* We are inside an EvalPlanQual recheck. Return the test
tuple
if
* one is available, after rechecking any
access-method-specific
* conditions.
*/
Index scanrelid = ((Scan *) node-ps.plan)-scanrelid;
Assert(scanrelid 0);
if (estate-es_epqTupleSet[scanrelid - 1])
{
TupleTableSlot *slot = node-ss_ScanTupleSlot;
:
return slot;
}
}
return (*accessMtd) (node);
}
When we are inside of EPQ, it fetches a tuple in es_epqTuple[] array and
checks its visibility (ForeignRecheck() always say 'yep, it is visible'),
then ExecScan() applies its qualifiers by ExecQual().
So, as long as FDW/CSP can return a record that satisfies the TupleDesc
of this relation, made by the tuples in es_epqTuple[] array, rest of the
code paths are common.
I have an idea to solve the problem.
It adds recheckMtd() call if scanrelid==0 just before the assertion
above,
and add a callback of FDW on
Re: [HACKERS] WIP: SCRAM authentication
On Wed, Aug 12, 2015 at 9:36 PM, Stephen Frost sfr...@snowman.net wrote: Yes, the SCRAM implementation could be buggy. But also, OpenSSL has repeatedly had security bugs that were due to forcing servers to downgrade to older protocols. I wouldn't like us to start growing similar vulnerabilities, where SCRAM would have been just fine but an attack that involves forcing a downgrade to MD5 is possible. I agree that such similar vulnerabilities would be unfortunate, but the way to avoid that is to not implement the actual hashing or encryption algorithms ourselves and to stick to the protocol as defined in the specification. Nothing in that will protect us if the client can request a non-SCRAM form of authentication. I don't think you are quite correct about the scenario where pg_authid is compromised. Even if the hash stored there is functionally equivalent to the password itself as far as this instance of PostgreSQL is concerned, the same password may have been used for other services, so cracking it has a purpose. I attempted to address that also by stating that, should an attacker compromise a system with the goal of gaining the cleartext password, they would attempt the following, in order: What if they steal a pg_dump? All of the password verifiers are there, but the live system is not. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Parsing tuple contents
Hello, I am working on a project which requires going through each field inside a tuple without using postgresql. I have managed to iterate through each tuple inside a table by recycling postgres's code. However, for the part of parsing through each field in the tuple, I am not able to think of anything other than using a bunch of if/else or switch case statements to handle each postgresql datatype. I looked through postgresql's code base but I am unable to identify the part of code that might do this. Could anyone please let me know where to look? Thanks, Vignesh
Re: [HACKERS] Test code is worth the space
* Michael Paquier (michael.paqu...@gmail.com) wrote: On Thu, Aug 13, 2015 at 5:54 AM, Stephen Frost wrote: The regression tests included in pgBackRest (available here: https://github.com/pgmasters/backrest) go through a number of different recovery tests. There's vagrant configs for a few different VMs too (CentOS 6, CentOS 7, Ubuntu 12.04 and Ubuntu 14.04) which is what we've been testing. We're working to continue expanding those tests and will also be adding tests for replication and promotion in the future. Eventually, we plan to write a buildfarm module for pgBackRest, to allow it to be run in the same manner as the regular buildfarm animals with the results posted. Interesting. Do you mind if I pick up from it some ideas for the in-core replication test suite based on TAP stuff? That's still in the works for the next CF. Certainly don't mind at all, entirely open source under the MIT license. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] [COMMITTERS] pgsql: Further fixes for degenerate outer join clauses.
Andrew Dunstan and...@dunslane.net writes: On 08/06/2015 03:36 PM, Tom Lane wrote: Further fixes for degenerate outer join clauses. Looks like this might have upset brolga on 9.0 and 9.1 - it's coming up with a different plan from what's expected. I looked into this, and while I can't be certain of the diagnosis without looking at the assembly code brolga generates (no, I don't want to), I'm pretty sure I know what's going on here. The two plans in question have exactly the same estimated costs on my machine, which results in add_path discarding the second one to arrive. Evidently, on brolga the second path gets some fractionally cheaper estimate due to some compiler-specific variation in the way the arithmetic is done, so it gets kept rather than the first one. In 9.2 and up we got rid of that class of problems with this patch: Author: Tom Lane t...@sss.pgh.pa.us Branch: master Release: REL9_2_BR [33e99153e] 2012-04-21 00:51:14 -0400 Use fuzzy not exact cost comparison for the final tie-breaker in add_path. Instead of an exact cost comparison, use a fuzzy comparison with 1e-10 delta after all other path metrics have proved equal. This is to avoid having platform-specific roundoff behaviors determine the choice when two paths are really the same to our cost estimators. Adjust the recently-added test case that made it obvious we had a problem here. but 9.0 and 9.1 still have platform-dependent treatment of paths with essentially equal cost estimates. I'm not entirely sure what to do about this. We could back-patch that patch into 9.0 and 9.1, but it's conceivable somebody would squawk about planner behavioral changes. The only other idea that seems practical is to remove regression test cases that have platform-specific results in those branches. Probably that wouldn't result in a real reduction in the quality of the test coverage for those branches (we could still execute the query, just not EXPLAIN it). But it seems like a pretty ad-hoc answer, and the next case might be one that hurts more not to test. 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] Macro nesting hell
Andres Freund and...@anarazel.de writes: On 2015-08-12 10:18:12 -0400, Tom Lane wrote: Sounds reasonable to me. If you do this, I'll see whether pademelon can be adjusted to build using the minimum macro expansion buffer size specified by the C standard. Here's the patch attached. Looks like you need to pay more attention to the surrounding comments: some of them still refer to the code as a macro, and I see at least one place that explicitly mentions double-eval hazards that this presumably removes. (I think your previous patch re fastgetattr was also a bit weak on the comments, btw.) 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] WIP: SCRAM authentication
* Michael Paquier (michael.paqu...@gmail.com) wrote: On Thu, Aug 13, 2015 at 10:22 AM, Stephen Frost wrote: The only case where I can see multiple verifiers per role making a real difference in migrations is for PGAAS hosting. But the folks from Heroku and AWS have been notably silent on this; lemme ping them. While their insight is certainly valuable, they are certainly not the only cases of one-user-to-rule-them-all environments. Further, there's going to be cases where multiple applications from different languages are accessing the database through the same account because there's only one account. I'd rather not punt on those cases and, further, assume that we'll always be able to keep it to only one password verifier per account. As I tried to outline up-thread, there are a set of features which would be very nice for us to have which require further information to be saved beyond even these different password verifiers for each. While looking at this stuff, I have been wondering as well about moving the validutil field into the verifier catalog as well for example. That's one. Agreed. As mentioned elsewhere, even SCRAM is possible of having multiple password verifiers based on the various algorithms used. In other words, I doubt the 'only one password verifier per role' approach is going to work out for us long term in any case. SCRAM is a definition for an authorization protocol which includes many verifiers, and the minimal requirement to consider that SCRAM is implemented in a system is to have SCRAM-SHA1, per here: http://tools.ietf.org/html/rfc5802 For example we may want to have in parallel one verifier for SCRAM-SHA1 and one for SCRAM-SHA256 for the same user, and I think that we cannot close the door either to other SASL protocols, which is why it may make sense to split the SCRAM patch into two with the basic message protocol infrastructure in place. and further agreed here. In addition, I'd point out that this is not a novel area in secure applications- Kerberos/GSSAPI have long been doing this, with both MIT Kerberos and Heimdal providing multiple algorithms and supporting multiple password verifiers per user. Active Directory also, actually. When it comes to enterprise authentication systems and administrators who work with them, this is not overly complex nor suprising. Thanks! Stephen signature.asc Description: Digital signature
[HACKERS] Our trial to TPC-DS but optimizer made unreasonable plan
(Please read this message on wide display) Our team recently tries to run TPC-DS benchmark to know capability of PostgreSQL towards typical analytic queries. TPC-DS defines about 100 complicated queries. We noticed optimizer made unreasonable execution plan towards some of queries. Here is an example (query.04) below, on bottom of this message. Its query execution plan by EXPLAIN is below. The most time consuming portion is multilevel nested-loop on bottom of the EXPLAIN output, because it sequentially runs on the CTE result. I cannot complete the query execution within 30minutes towards SF=1 on Xeon E5-2670v3 and RAM=384GB environment. -- Limit (cost=1248808.70..1248808.71 rows=1 width=132) CTE year_total - Append (cost=193769.66..496076.44 rows=4778919 width=220) - HashAggregate (cost=193769.66..226692.26 rows=2633808 width=178) Group Key: customer.c_customer_id, customer.c_first_name, customer.c_last_name, customer.c_preferred_cust_flag, customer.c_birth_country, customer.c_login, customer.c_email_address, date_dim.d_year - Custom Scan (GpuJoin) (cost=14554.84..108170.90 rows=2633808 width=178) Bulkload: On (density: 100.00%) Depth 1: Logic: GpuHashJoin, HashKeys: (ss_sold_date_sk), JoinQual: (ss_sold_date_sk = d_date_sk), nrows_ratio: 0.95623338 Depth 2: Logic: GpuHashJoin, HashKeys: (ss_customer_sk), JoinQual: (ss_customer_sk = c_customer_sk), nrows_ratio: 0.91441411 - Custom Scan (BulkScan) on store_sales (cost=0.00..96501.23 rows=2880323 width=38) - Seq Scan on date_dim (cost=0.00..2705.49 rows=73049 width=16) - Seq Scan on customer (cost=0.00..4358.00 rows=10 width=156) - HashAggregate (cost=125474.72..143301.10 rows=1426111 width=181) Group Key: customer_1.c_customer_id, customer_1.c_first_name, customer_1.c_last_name, customer_1.c_preferred_cust_flag, customer_1.c_birth_country, customer_1.c_login, customer_1.c_email_address, date_dim_1.d_year - Custom Scan (GpuJoin) (cost=14610.07..79126.11 rows=1426111 width=181) Bulkload: On (density: 100.00%) Depth 1: Logic: GpuHashJoin, HashKeys: (cs_bill_customer_sk), JoinQual: (c_customer_sk = cs_bill_customer_sk), nrows_ratio: 0.99446636 Depth 2: Logic: GpuHashJoin, HashKeys: (cs_sold_date_sk), JoinQual: (cs_sold_date_sk = d_date_sk), nrows_ratio: 0.98929483 - Custom Scan (BulkScan) on catalog_sales (cost=0.00..65628.43 rows=1441543 width=41) - Seq Scan on customer customer_1 (cost=0.00..4358.00 rows=10 width=156) - Seq Scan on date_dim date_dim_1 (cost=0.00..2705.49 rows=73049 width=16) - HashAggregate (cost=69306.38..78293.88 rows=719000 width=181) Group Key: customer_2.c_customer_id, customer_2.c_first_name, customer_2.c_last_name, customer_2.c_preferred_cust_flag, customer_2.c_birth_country, customer_2.c_login, customer_2.c_email_address, date_dim_2.d_year - Custom Scan (GpuJoin) (cost=14702.18..45938.88 rows=719000 width=181) Bulkload: On (density: 100.00%) Depth 1: Logic: GpuHashJoin, HashKeys: (ws_bill_customer_sk), JoinQual: (c_customer_sk = ws_bill_customer_sk), nrows_ratio: 0.99973309 Depth 2: Logic: GpuHashJoin, HashKeys: (ws_sold_date_sk), JoinQual: (ws_sold_date_sk = d_date_sk), nrows_ratio: 0.99946618 - Custom Scan (BulkScan) on web_sales (cost=0.00..32877.84 rows=719384 width=41) - Seq Scan on customer customer_2 (cost=0.00..4358.00 rows=10 width=156) - Seq Scan on date_dim date_dim_2 (cost=0.00..2705.49 rows=73049 width=16) - Sort (cost=752732.27..752732.27 rows=1 width=132) Sort Key: t_s_secyear.customer_id, t_s_secyear.customer_first_name, t_s_secyear.customer_last_name, t_s_secyear.customer_email_address - Nested Loop (cost=0.00..752732.26 rows=1 width=132) Join Filter: ((t_s_secyear.customer_id = t_w_secyear.customer_id) AND (CASE WHEN (t_c_firstyear.year_total '0'::numeric) THEN (t_c_secyear.year_total / t_c_firstyear.year_total) ELSE NULL::numeric END CASE WHEN (t_w_firstyear.year_total '0'::numeric) THEN (t_w_secyear.year_total / t_w_firstyear.year_total) ELSE NULL::numeric END)) - Nested Loop (cost=0.00..633256.31 rows=1 width=308) Join Filter: ((t_s_secyear.customer_id = t_c_secyear.customer_id) AND (CASE WHEN (t_c_firstyear.year_total '0'::numeric) THEN (t_c_secyear.year_total / t_c_firstyear.year_total) ELSE NULL::numeric END CASE WHEN
Re: [HACKERS] WIP: SCRAM authentication
On Thu, Aug 13, 2015 at 6:21 AM, Josh Berkus wrote: The only case where I can see multiple verifiers per role making a real difference in migrations is for PGAAS hosting. But the folks from Heroku and AWS have been notably silent on this; lemme ping them. Yes, I would be curious to hear from their requirements in this area. I think it is actually a big deal for those folks to be able to get multiple verifiers living in parallel for one role. -- 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] [sqlsmith] ERROR: failed to build any %d-way joins
Andreas Seltenreich seltenre...@gmx.de writes:
there's a 1/1e6 chance that a sqlsmith query on the regression db of
master (c124cef) fails with
ERROR: failed to build any {4..8}-way joins
Looks like I broke that while trying to fix one of your previous
reports :-(. I think it's all better now, but your continued testing
is much appreciated.
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] Test code is worth the space
* Robert Haas (robertmh...@gmail.com) wrote: On Wed, Aug 12, 2015 at 9:24 PM, Stephen Frost sfr...@snowman.net wrote: * Michael Paquier (michael.paqu...@gmail.com) wrote: On Thu, Aug 13, 2015 at 5:54 AM, Stephen Frost wrote: The regression tests included in pgBackRest (available here: https://github.com/pgmasters/backrest) go through a number of different recovery tests. There's vagrant configs for a few different VMs too (CentOS 6, CentOS 7, Ubuntu 12.04 and Ubuntu 14.04) which is what we've been testing. We're working to continue expanding those tests and will also be adding tests for replication and promotion in the future. Eventually, we plan to write a buildfarm module for pgBackRest, to allow it to be run in the same manner as the regular buildfarm animals with the results posted. Interesting. Do you mind if I pick up from it some ideas for the in-core replication test suite based on TAP stuff? That's still in the works for the next CF. Certainly don't mind at all, entirely open source under the MIT license. Why not the PG license? It would be nicer if we didn't have to worry about license contamination here. There is no conflict between the licenses and pgBackRest is not part of nor maintained as part of the PostgreSQL project. Not that I'm against that changing, certainly, but it was independently developed and I wouldn't ask the community to take on maintaining something which wasn't developed following the community process, not to mention that I imagine some would probably want it rewritten in C. We clearly have dependencies, to the extent that there's concern about licenses, on much more restrictive and interesting licenses, and there currently isn't even any real dependency here and would only be if the PGDG decided to fork pgBackRest and start maintaining it independently. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Parsing tuple contents
Thank you very much for the response. On Wed, Aug 12, 2015 at 6:31 PM, Tom Lane t...@sss.pgh.pa.us wrote: Vignesh Raghunathan vignesh.pg...@gmail.com writes: I am working on a project which requires going through each field inside a tuple without using postgresql. I have managed to iterate through each tuple inside a table by recycling postgres's code. However, for the part of parsing through each field in the tuple, I am not able to think of anything other than using a bunch of if/else or switch case statements to handle each postgresql datatype. I looked through postgresql's code base but I am unable to identify the part of code that might do this. Could anyone please let me know where to look? Well, as far as identifying the field boundaries is concerned, there are not that many cases: you basically only need to worry about typlen and typalign. heap_deform_tuple() would be a good model. Of course, if you want to print the values in some human-readable form, there is not going to be a good substitute for per-datatype code :-( regards, tom lane
Re: [HACKERS] Raising our compiler requirements for 9.6
On 2015-08-08 02:30:44 -0400, Noah Misch wrote: On Sat, Aug 08, 2015 at 02:30:47AM +0200, Andres Freund wrote: On 2015-08-07 20:16:20 -0400, Noah Misch wrote: I agree that lock.h offers little to frontend code. Headers that the lockdefs.h patch made usable in the frontend, particularly genam.h and hash.h, are no better. It's not that simple. Those two, and tuptoaster.h, are actually somewhat validly included by frontend code via the rmgr descriptor routines. genam.h is a dependency of the non-frontend-relevant content of some frontend-relevant headers, _exactly_ as lock.h has been. I count zero things in genam.h that a frontend program could harness. The frontend includes hash.h for two hashdesc.c prototypes, less than the material you moved out of lock.h for frontend benefit. Yes, it is that simple. The point is that it's included from various headers and that there's really no need to include lock.h from a header that just wants to declare a LOCKMODE as it's parameter. There's no other contents in lock.h afaics that fit the billet similarly. To me it's a rather worthwhile goald to want *am.h not to pull in details of the locking implementations, but they're going to have to define a LOCKMODE parameter and the values passed in. We're not entirely there, but it's not much further work. We could split some stuff in a more 'locking internals' oriented headers (PROC_QUEUE, LockMethodData and dependants, LOCKTAG and depenedants), but afaics it'd not be a proper lock_internal.h header either, because there's code like index.c that currently does SET_LOCKTAG_RELATION itself. The lock.h/lockdefs.h unprincipled split would do more harm than letting frontends continue to pull in lock.h. Why? Your header comment for lockdefs.h sums up the harm nicely. Additionally, the term defs does nothing to explain the split. lock2.h would be no less evocative. Oh, come on. It's not the only header that's split that way (see xlogdefs.h). Splitting away some key definitions so not all the internals are dragged in when needing just the simplest definitions is not an absurd concept. Consider what happens when lock.h/c gets more complicated and e.g. grows some atomics. None of the frontend code should see that, and it's not much effort to keep it that way. Allowing client code to see LOCKMODE isn't something that's going to cause any harm. Readying the headers for that day brings some value, but you added a worse mess to achieve it. The overall achievement has negative value. I honestly can't follow. Why is it so absurd to avoid including lock.h from more code, including FRONTEND one? Or is it just the lockdefs.h split along the 'as-currently-required' line that you object to? 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] How to compare different datums within from a tuple?
Am 11.08.2015 um 21:03 schrieb Peter Eisentraut:
On 8/10/15 12:36 PM, Peter Moser wrote:
Can someone tell me, how I can compare two datum fields, when I do not
know the data type in advance inside an executor function?
For example, x less than y where x and y are of various types that
form intervals. I have found the method ExecTuplesMatch, but it is only
for equality comparison, I think. Another one is ApplySortComparator...
maybe that's the correct way to go?
Some code to make things clearer...
Datum x = heap_getattr(out-tts_tuple,
node-xpos,
out-tts_tupleDescriptor,
isNull1);
Datum y = slot_getattr(curr, node-ypos, isNull2);
if (compareDatumWithCorrectMethod(x,y) 0)
{
/* do something */
}
The tuple descriptor will contain the data type of the datum, so you can
use that to look up the default btree operator class and call the
respective operators in there. But note that there is no single notion
of comparison in the system. Comparison depends on operator class,
access method, possibly collation, null value treatment. Some types
don't support comparison beyond equality. A robust patch would need to
take that into account.
Ok, thank you.
Now I have a first solution. I am just wondering if this is robust, or
do I miss something? Thanks for any comments...
My executor consumes rows from my own rewritten sub-query. From this
sub-query I extract one sortGroupClause and from that the eqop and
sortop during planning.
sgc = (SortGroupClause *) llast(sortClause);
node-eqOperator = sgc-eqop;
node-ltOperator = sgc-sortop;
The last sort clause uses the same types as the executor needs to
compare later.
The executor initializes the methods with:
state-ltFunctionInfo = (FmgrInfo *) palloc(sizeof(FmgrInfo));
ltOperatorId = get_opcode(node-ltOperator);
fmgr_info(ltOperatorId, state-ltFunctionInfo);
state-eqFunctionInfo = (FmgrInfo *) palloc(sizeof(FmgrInfo));
eqOperatorId = get_opcode(node-eqOperator);
fmgr_info(eqOperatorId, state-eqFunctionInfo);
Finally I use them in this way...
static bool
isLessThan(Datum a, Datum b, FmgrInfo *ltFunctionInfo)
{
return DatumGetBool(FunctionCall2(ltFunctionInfo, a, b));
}
static bool
isEqual(Datum a, Datum b, FmgrInfo *eqFunctionInfo)
{
return DatumGetBool(FunctionCall2(eqFunctionInfo, a, b));
}
--
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] Test code is worth the space
On 12 August 2015 at 03:10, Noah Misch n...@leadboat.com wrote: On another review I suggested we add a function to core to allow it to be used in regression tests. A long debate ensued, deciding that we must be consistent and put diagnostic functions in contrib. My understanding is that we are not allowed to write regression tests that use contrib modules, yet the consistent place to put diagnostic functions is contrib - therefore, we are never allowed to write tests utilizing diagnostic functions. We are allowed to put modules for testing in another directory, but these are not distributed to users so cannot be used for production diagnosis. Systemic fail, advice needed. If you want a user-visible function for production diagnosis, set aside the fact that you wish to use it in a test, and find the best place for it. Then, place the tests with the function. (That is, if the function belongs in contrib for other reasons, put tests calling it in the contrib module itself.) Place non-user-visible test support functions in regress.c, or use one of the options Robert described. That helps, thanks. -- Simon Riggshttp://www.2ndQuadrant.com/ http://www.2ndquadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services
Re: [HACKERS] PL/pgSQL, RAISE and error context
2015-08-10 18:43 GMT+02:00 Pavel Stehule pavel.steh...@gmail.com: 2015-08-10 9:11 GMT+02:00 Heikki Linnakangas hlinn...@iki.fi: On 07/26/2015 08:34 AM, Pavel Stehule wrote: Hi here is complete patch, that introduce context filtering on client side. The core of this patch is trivial and small - almost all of size are trivial changes in regress tests - removing useless context. Documentation, check-world Looks good to me at first glance. I'll mark this as Ready for Committer. Is it acceptable for all? I have not a problem with this way. So, there is common agreement on this version. Best regards Pavel Regards Pavel - Heikki
Re: [HACKERS] Macro nesting hell
On 2015-07-01 12:55:48 -0300, Alvaro Herrera wrote: Tom Lane wrote: Last night my ancient HP compiler spit up on HEAD: http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=pademelondt=2015-07-01%2001%3A30%3A18 complaining thus: cpp: brin_pageops.c, line 626: error 4018: Macro param too large after substitution - use -H option. I was able to revive pademelon by adding a new compiler flag as suggested, but after looking at what the preprocessor is emitting, I can't say that I blame it for being unhappy. This simple-looking line Assert(BRIN_IS_REGULAR_PAGE(BufferGetPage(oldbuf))); is expanding to this: I just hit this in clang which also warns about too long literals unless you silence it... Wow, that's kind of amazing. I think this particular case boils down to just PageGetSpecialPointer (bufpage.h) and BufferGetBlock (bufmgr.h). Inlining just BufferGetBlock already helps sufficiently to press it below 4k (the standard's limit IIRC), but that doesn't mean we shouldn't go a bit further. I'm thinking we really ought to mount a campaign to replace some of these macros with inlined-if-possible functions. My guess is that changing a very small amount of them will do a large enough portion of the job. I think it'd be good to convert the macros in bufpage.h and bufmgr.h that either currently have multiple evaluation hazards, or have a AssertMacro() in them. The former for obvious reasons, the latter because that immediately makes them rather large (on and it implies multiple evaluation hazards anyway). That'd mean inlining PageSetPageSizeAndVersion(), PageGetSpecialSize(), PageGetSpecialPointer(), PageGetItem(), PageGetMaxOffsetNumber(), PageIsPrunable(), PageSetPrunable(), PageClearPrunable(), BufferIsValid(), BufferGetBlock(), BufferGetPageSize(). 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] optimizing vacuum truncation scans
On 3 August 2015 at 17:18, Jeff Janes jeff.ja...@gmail.com wrote: That does still leave the prefetch technique, so all is not lost. Can we see a patch with just prefetch, probably with a simple choice of stride? Thanks. I probably won't get back to it this commit fest, so it can be set to returned with feedback. But if anyone has good ideas for how to set the stride (or detect that it is on SSD and so is pointless to try) I'd love to hear about them anytime. I've Returned With Feedback, as you suggest. Given your earlier numbers, I'd just pick a constant value of 128, to keep it simple. That balances out the various factors of small/large tables and the uncertainty that we will be able to truncate the whole chunk of blocks. I'd like to see tests on SSD before commit, but I hope and expect the slightly negative results with SSD won't be substantiated on larger tests. Kept simple, its a patch with a clear win in a restricted use case and I would be happy to commit that sometime in the future. -- Simon Riggshttp://www.2ndQuadrant.com/ http://www.2ndquadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services
Re: [HACKERS] patch : Allow toast tables to be moved to a different tablespace
On 3 August 2015 at 01:35, Andreas Karlsson andr...@proxel.se wrote: On 07/15/2015 09:30 PM, Robert Haas wrote: On Tue, Jul 14, 2015 at 5:57 PM, Jim Nasby jim.na...@bluetreble.com wrote: On 7/7/15 7:07 AM, Andres Freund wrote: On 2015-07-03 18:03:58 -0400, Tom Lane wrote: I have just looked through this thread, and TBH I think we should reject this patch altogether --- not RWF, but no we don't want this. The use-case remains hypothetical: no performance numbers showing a real-world benefit have been exhibited AFAICS. It's not that hard to imagine a performance benefit though? If the toasted column is accessed infrequently/just after filtering on other columns (not uncommon) it could very well be beneficial to put the main table on fast storage (SSD) and the toast table on slow storage (spinning rust). I've seen humoungous toast tables that are barely ever read for tables that are constantly a couple times in the field. +1. I know of one case where the heap was ~8GB and TOAST was over 400GB. Yeah, I think that's a good argument for this. I have to admit that I'm a bit fatigued by this thread - it started out as a learn PostgreSQL project, and we iterated through a few design problems that made me kind of worried about what sort of state the patch was in - and now this patch is more than 4 years old. But if some committer still has the energy to go through it in detail and make sure that all of the problems have been fixed and that the patch is, as Andreas says, in good shape, then I don't see why we shouldn't take it. I personally think the patch is in a decent shape, and a worthwhile feature. I agree though with Tom's objections about the pg_dump code. I do not have enough time or interest right now to fix up this patch (this is not a feature I personally have a lot of interest in), but if someone else wishes to I do not think it would be too much work. To me the patch looks like it is in reasonable shape, caveats noted. The patch doesn't really take us very far forwards in terms of administration since workarounds exist which people use and understand. Noting Tom's comments on additional complexity it seems like it could be a source of bugs or production problems. The deciding factor is that it brings TOAST as a keyword for us to support forever. While reviewing this, I've come to realise that a better approach to column stores and/or vertical partitioning is the more general way of handling this, so creating a support legacy at this time doesn't make sense to me personally. On balance, the negatives seem to outweigh the positives so isn't the best use of my time to fix it up. I'm now returning this with feedback. -- Simon Riggshttp://www.2ndQuadrant.com/ http://www.2ndquadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services
[HACKERS] Warnings around booleans
Hi,
Forcing our bool to be stdbool.h shows up a bunch of errors and
warnings:
1) gin stores/queries some bools as GinTernaryValue.
Part of this is easy to fix, just adjust GinScanKeyData-entryRes to
be a GinTernaryValue (it's actually is compared against MAYBE).
What I find slightly worrysome is that in gin_tsquery_consistent()
checkcondition_gin (returning GinTernaryValue) is passed as a
callback that's expected to return bool. And the field
checkcondition_gin is returning (GinChkVal-check[i]) actually is a
ternary.
2) help_config.c has a member named 'bool' in a local struct. That
doesn't work well if bool actually is a macro. Which mean that whole
#ifndef bool patch in c.h wasn't exercised since 2003 when
help_config.c was created...
= rename field to bool_ (or _bool but that looks a wee close to C's _Bool)
3) The 'bypassrls' boolean in AlterUser() is queried strangely. There are two
complaints:
boolbypassrls = -1;
it's a boolean.
else if (authform-rolbypassrls || bypassrls = 0)
{
if (!superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg(must be superuser to change bypassrls
attribute)));
}
...
if (bypassrls = 0)
{
new_record[Anum_pg_authid_rolbypassrls - 1] =
BoolGetDatum(bypassrls 0);
new_record_repl[Anum_pg_authid_rolbypassrls - 1] = true;
}
if it's a boolean, that's always true.
It's not entirely clear to me why this was written that way. Stephen?
3) vacuumdb.c stores -1's in a bool (result) in vacuum_one_database().
= convert to an int
4) _tableInfo-relreplident is a bool, should be a char
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] Raising our compiler requirements for 9.6
On 2015-08-11 22:34:40 -0400, Noah Misch wrote: On Tue, Aug 11, 2015 at 01:04:48PM +0200, Andres Freund wrote: On 2015-08-05 15:46:36 +0200, Andres Freund wrote: On 2015-08-05 15:08:29 +0200, Andres Freund wrote: We might later want to change some of the harder to maintain macros to inline functions, but that seems better done separately. Here's a conversion for fastgetattr() and heap_getattr() Slightly updated version attached. In my opinion this drastically increases readability and thus should be applied. Will do so sometime tomorrow unless there's protest. -1 to introducing more inline functions before committable code replaces what you've already pushed for this thread. Seriously? I've no problem with fixing anything. So far we have don't seem to have to come to a agreement what exactly that fix would be. Tom has stated that he doesn't want lock.h made smaller on account of frontend code including it, and you see that as the right way. 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] PL/pgSQL, RAISE and error context
On 8/12/15 9:36 AM, Pavel Stehule wrote: So, there is common agreement on this version. There are several instances of double semicolons. Also, PsqlSettings.show_context doesn't look like a boolean to me. For SHOW_CONTEXT, it would be good if the documentation mentioned the default value. I'm somewhat worried that this is hiding important context from some NOTICE or WARNING messages intended for novice users, but probably not worried enough to go through all of them. +3/8 from me, I guess. .m -- 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] PL/pgSQL, RAISE and error context
2015-08-12 11:07 GMT+02:00 Marko Tiikkaja ma...@joh.to: On 8/12/15 9:36 AM, Pavel Stehule wrote: So, there is common agreement on this version. There are several instances of double semicolons. Also, PsqlSettings.show_context doesn't look like a boolean to me. For SHOW_CONTEXT, it would be good if the documentation mentioned the default value. I'm somewhat worried that this is hiding important context from some NOTICE or WARNING messages intended for novice users, but probably not worried enough to go through all of them. +3/8 from me, I guess. Thank you for info I'll fix it .m
Re: [HACKERS] Warnings around booleans
On 2015-08-12 10:43:51 +0200, Andres Freund wrote:
3) The 'bypassrls' boolean in AlterUser() is queried strangely. There are two
complaints:
boolbypassrls = -1;
it's a boolean.
else if (authform-rolbypassrls || bypassrls = 0)
{
if (!superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg(must be superuser to change bypassrls
attribute)));
}
...
if (bypassrls = 0)
{
new_record[Anum_pg_authid_rolbypassrls - 1] =
BoolGetDatum(bypassrls 0);
new_record_repl[Anum_pg_authid_rolbypassrls - 1] = true;
}
if it's a boolean, that's always true.
It's not entirely clear to me why this was written that
way. Stephen?
Which incidentally leads to really scary results:
postgres[25069][1]=# SELECT oid, rolname, rolbypassrls FROM pg_authid WHERE
rolname = 'plain';
┌───┬─┬──┐
│ oid │ rolname │ rolbypassrls │
├───┼─┼──┤
│ 76892 │ plain │ f│
└───┴─┴──┘
(1 row)
postgres[25069][1]=# ALTER ROLE plain NOLOGIN;
ALTER ROLE
postgres[25069][1]=# SELECT oid, rolname, rolbypassrls FROM pg_authid WHERE
rolname = 'plain';
┌───┬─┬──┐
│ oid │ rolname │ rolbypassrls │
├───┼─┼──┤
│ 76892 │ plain │ t│
└───┴─┴──┘
(1 row)
The -1 isn't a valid value for a bool, which is now unsigned, and just
wraps around to true.
There are no tests catching this, which is a scary in it's own right.
So on linux/x86 this normally is only an issue when using a definition
for bool other than c.h's (which we explicitly try to cater for!). But
on other platforms the whole logic above is terminally broken: Consider
what happens when char is unsigned. That's e.g. the case on nearly all,
if not all, arm compilers.
This is reproducible today in an unmodified postgres on x86 if you add
-funsigned-char. Or, on any arm and possibly some other platforms.
Whoa, allowing the compiler to warn us is a good thing. This would have
been a fucktastically embarassing security issue.
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] How to compare different datums within from a tuple?
Am 11.08.2015 um 13:41 schrieb Anastasia Lubennikova:
Can someone tell me, how I can compare two datum fields, when I do
not know the data type in advance inside an executor function?
In a nutshell, there is no way to compare Datums.
Datum is an abstact data type. It's the backend internal representation
of a single value of any SQL data type.
The code using Datum has to know which type it is, since the Datum
itself doesn't contain that information.
For example, x less than y where x and y are of various types that
form intervals. I have found the method ExecTuplesMatch, but it is
only for equality comparison, I think. Another one is
ApplySortComparator... maybe that's the correct way to go?
Some code to make things clearer...
Datum x = heap_getattr(out-tts_tuple,
node-xpos,
out-tts_tupleDescriptor,
isNull1);
Datum y = slot_getattr(curr, node-ypos, isNull2);
if (compareDatumWithCorrectMethod(x,y) 0)
{
/* do something */
}
Thx,
Peter
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org
mailto:pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Maybe you can use DatumGetXXX function to extract value. For example,
DatumGetInt32.
http://doxygen.postgresql.org/postgres_8h.html#aacbc8a3ac6d52d85feaf0b7ac1b1160c
--
Best regards,
Lubennikova Anastasia
I did this with another column, that has always the result of
row_number() as content, which I think will be fine when I use
DatumGetInt32.
Thanks,
Peter
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Tab completion for CREATE SEQUENCE
On Wed, Aug 12, 2015 at 9:22 AM, Vik Fearing v...@2ndquadrant.fr wrote: Thank you, Brendan, Michael, and Robert for taking care of this while I was away. Thanks for the patch! -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On 2015-08-11 13:49:19 -0300, Alvaro Herrera wrote: Andres Freund wrote: On 2015-08-11 12:43:03 -0400, Robert Haas wrote: On Tue, Aug 11, 2015 at 12:40 PM, Tom Lane t...@sss.pgh.pa.us wrote: We do not use !! elsewhere for this purpose, and I for one find it a pretty ugly locution. We do, actually, now, in contrib/pg_xlogdump/pg_xlogdump.c. I'd be in favor of getting rid of those. Those got already removed by Heikki's WAL format changes... And a bunch more places actually. Blame me. I'll come up with a patch fixing the macros and converting existing !! style ones. Actually there's one that's not yours ... I'm not touching sepgsql... ;) I went through all headers in src/include and checked for macros containing [^][^] and checked whether they have this hazard. Found a fair number. That patch also changes !! tests into != 0 style. Greetings, Andres Freund From 2c67e83b945ff9183dd3b5cdeea78f4bde5b8a74 Mon Sep 17 00:00:00 2001 From: Andres Freund and...@anarazel.de Date: Wed, 12 Aug 2015 16:05:35 +0200 Subject: [PATCH] Return only 0/1 from boolean macros. Previously some test macros returned the result of bit operations. That obviously works without problems when used as part of a boolean expression - but if e.g. the result is assigned to a boolean variable the result in some cases can be less than pretty. Depending on the source of the type definition for bool the result might be too wide or, if going through a pointer, the result can be an invalid value for a boolean (stdbool.h's bool e.g. only allows 0 and 1 values). The latter e.g. causes test failures with gcc when using a stdbool.h type boolean. As discussed also change some existing macros that forced a 0/1 return value by using !! to != 0 comparisons. I tried to find all relevant test macros in headers, there very well might be additional ones in C files themselves. Discussion: 20150811154237.gd17...@awork2.anarazel.de --- src/backend/access/transam/xact.c | 3 ++- src/backend/replication/logical/reorderbuffer.c | 2 +- src/backend/storage/lmgr/lwlock.c | 6 +++--- src/include/access/gin_private.h| 16 src/include/access/gist.h | 8 src/include/access/htup_details.h | 8 src/include/access/itup.h | 4 ++-- src/include/access/nbtree.h | 14 +++--- src/include/access/spgist_private.h | 10 +- src/include/access/xact.h | 4 ++-- src/include/catalog/pg_trigger.h| 10 +- src/include/commands/trigger.h | 2 +- src/include/regex/regguts.h | 2 +- src/include/storage/bufpage.h | 6 +++--- src/include/utils/jsonb.h | 6 +++--- 15 files changed, 51 insertions(+), 50 deletions(-) diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index b53d95f..f5ec79f 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -5310,7 +5310,8 @@ xact_redo_commit(xl_xact_parsed_commit *parsed, LWLockRelease(XidGenLock); } - Assert(!!(parsed-xinfo XACT_XINFO_HAS_ORIGIN) == (origin_id != InvalidRepOriginId)); + Assert(((parsed-xinfo XACT_XINFO_HAS_ORIGIN) != 0) == + (origin_id != InvalidRepOriginId)); if (parsed-xinfo XACT_XINFO_HAS_ORIGIN) commit_time = parsed-origin_timestamp; diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c index cdc7bd7..6664baa 100644 --- a/src/backend/replication/logical/reorderbuffer.c +++ b/src/backend/replication/logical/reorderbuffer.c @@ -575,7 +575,7 @@ ReorderBufferTXNByXid(ReorderBuffer *rb, TransactionId xid, bool create, if (is_new) *is_new = !found; - Assert(!create || !!txn); + Assert(!create || txn != NULL); return txn; } diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c index 687ed63..99a1d3c 100644 --- a/src/backend/storage/lmgr/lwlock.c +++ b/src/backend/storage/lmgr/lwlock.c @@ -190,11 +190,11 @@ PRINT_LWDEBUG(const char *where, LWLock *lock, LWLockMode mode) errmsg(%d: %s(%s %d): excl %u shared %u haswaiters %u waiters %u rOK %d, MyProcPid, where, T_NAME(lock), T_ID(lock), - !!(state LW_VAL_EXCLUSIVE), + (state LW_VAL_EXCLUSIVE) != 0, state LW_SHARED_MASK, - !!(state LW_FLAG_HAS_WAITERS), + (state LW_FLAG_HAS_WAITERS) != 0, pg_atomic_read_u32(lock-nwaiters), - !!(state LW_FLAG_RELEASE_OK; + (state LW_FLAG_RELEASE_OK) != 0))); } } diff --git a/src/include/access/gin_private.h b/src/include/access/gin_private.h index 5095fc1..1b63ead 100644 --- a/src/include/access/gin_private.h +++ b/src/include/access/gin_private.h @@ -112,22 +112,22 @@ typedef struct GinMetaPageData */ #define GinPageGetOpaque(page) (
Re: [HACKERS] WIP: SCRAM authentication
On Wed, Aug 12, 2015 at 10:50 AM, Peter Eisentraut pete...@gmx.net wrote: I understand this idea, but I think it's not practical for many uses. There is no way to find out, on the server, whether all current clients would support a switch to SCRAM. Let alone all not-current clients. The only way to do such a switch would be to do the switch and then check for connection failures in the log, which is not good. Well, number one, I don't think we log anything currently that would let you figure that out; and number two, I'm not sure I believe that's the only way to make sure you've updated your clients. It would be better if we allowed both methods side by side. Then an administrator can check in the logs which clients are using an old method and track those down without interrupting production. (Now that I think about this, to counter my point, this is very similar to the switch from crypt to md5. You couldn't turn that on until you were sure that all clients would support it. I don't remember the experience from back then, though.) Maybe we should try to find out how that played out. It could inform the current discussion. Also, correct me if I'm wrong, but I believe using SCRAM would also make it harder to use the password exchange sniffed from the wire. So there might be a benefit to using SCRAM even if you have to keep old and supposedly insecure md5 hashes around for a while. Yeah. I guess there's the scenario where you use SCRAM with clients outside the firewall and MD5 with clients inside the firewall. But, meh. For every person who benefits from the ability to configure things that way, there will be 3 or 4 or 10 who enable SCRAM and never get rid of their old password verifiers. That will open up a vulnerability for people to attack the old verifiers, or perhaps allow some kind of attack where they can triangulate based on knowing that the MD5 verifiers and the SCRAM verifier are based on the same underlying password. Another thing we might want to try to find out is: if we add SCRAM authentication to 9.6, how committed are drivers authors to adding that support to their drivers? If we poll the maintainers of the drivers for Perl, Python, Ruby, Node.JS, Java, ODBC, etc. and involve them in this conversation, we might learn useful things. This is a big change we're talking about, and it's only to work (regardless of the details) if the driver authors are on board. We haven't, AFAIK, talked to them about this at all. -- 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] Raising our compiler requirements for 9.6
On Tue, Aug 11, 2015 at 10:34 PM, Noah Misch n...@leadboat.com wrote: In my opinion this drastically increases readability and thus should be applied. Will do so sometime tomorrow unless there's protest. -1 to introducing more inline functions before committable code replaces what you've already pushed for this thread. This appears to be intended as an insult, but maybe I'm misreading it. I am not thrilled with the rate at which this stuff is getting whacked around. Less-significant changes have been debated for far longer, and I really doubt that the rate at which Andres is committing changes in this area is healthy. I don't doubt that he has good intentions, though. -- 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] Warnings around booleans
On 2015-08-12 08:16:09 -0400, Stephen Frost wrote:
1) gin stores/queries some bools as GinTernaryValue.
Part of this is easy to fix, just adjust GinScanKeyData-entryRes to
be a GinTernaryValue (it's actually is compared against MAYBE).
What I find slightly worrysome is that in gin_tsquery_consistent()
checkcondition_gin (returning GinTernaryValue) is passed as a
callback that's expected to return bool. And the field
checkcondition_gin is returning (GinChkVal-check[i]) actually is a
ternary.
Is there a potential corruption issue from that..?
I honestly don't understand the gin code well enough to answer that.
2) help_config.c has a member named 'bool' in a local struct. That
doesn't work well if bool actually is a macro. Which mean that whole
#ifndef bool patch in c.h wasn't exercised since 2003 when
help_config.c was created...
= rename field to bool_ (or _bool but that looks a wee close to C's
_Bool)
I don't particularly like just renaming it to bool_, for my part, but
certainly need to do something.
There's alread a _enum in the struct, so that doesn't bother my much.
3) The 'bypassrls' boolean in AlterUser() is queried strangely. There are
two
complaints:
I assume you mean AlterRole() above?
Oops.
boolbypassrls = -1;
it's a boolean.
else if (authform-rolbypassrls || bypassrls = 0)
{
if (!superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg(must be superuser to change bypassrls
attribute)));
}
...
if (bypassrls = 0)
{
new_record[Anum_pg_authid_rolbypassrls - 1] =
BoolGetDatum(bypassrls 0);
new_record_repl[Anum_pg_authid_rolbypassrls - 1] = true;
}
if it's a boolean, that's always true.
It's not entirely clear to me why this was written that way. Stephen?
I specifically recall fixing a similar issue in this area but clearly
didn't realize that it was a bool when, as I recall, I was changing it
to match what we do for all of the other role attributes. In those
other cases the value is an int, not a bool though. Changing it to an
int here should make it line up with the rest of AlterRole().
I find that a somewhat ugly coding pattern, but since the rest of the
function is written that way...
I'll do that and add regression tests for it and any others which don't
get tested. My thinking on the test is to independently change each
value and then poll for all role attributes set and verify that the only
change made was the change expected.
Do that if you like, but what I really think we should have is a test
that tries to bypass rls and fails, then the user gets changes and it
succeeds, and then it gets disabled and fails again. This really seems
test-worthy behaviour to me.
3) vacuumdb.c stores -1's in a bool (result) in vacuum_one_database().
= convert to an int
Perhaps I'm missing something, but it appears to only ever be set to 0
or -1, so wouldn't it make sense to just correct it to use bool properly
instead?
Yea, you're right.
4) _tableInfo-relreplident is a bool, should be a char
This results in pg_dump always failing to dump out the necessary
ALTER TABLE ONLY ... REPLICA IDENTITY ... when it's actually required,
right? Is that something we can add a regression test to cover which
will be exercised through the pg_upgrade path?
With our current boolean definition this doesn't fail at all. All the
values fall into both char and unsigned char range.
WRT tests: It'd probably be a good idea to not drop the tables at the
end of replica_identity.sql.
--
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] Warnings around booleans
Andres,
* Andres Freund (and...@anarazel.de) wrote:
Forcing our bool to be stdbool.h shows up a bunch of errors and
warnings:
Wow.
1) gin stores/queries some bools as GinTernaryValue.
Part of this is easy to fix, just adjust GinScanKeyData-entryRes to
be a GinTernaryValue (it's actually is compared against MAYBE).
What I find slightly worrysome is that in gin_tsquery_consistent()
checkcondition_gin (returning GinTernaryValue) is passed as a
callback that's expected to return bool. And the field
checkcondition_gin is returning (GinChkVal-check[i]) actually is a
ternary.
Is there a potential corruption issue from that..?
2) help_config.c has a member named 'bool' in a local struct. That
doesn't work well if bool actually is a macro. Which mean that whole
#ifndef bool patch in c.h wasn't exercised since 2003 when
help_config.c was created...
= rename field to bool_ (or _bool but that looks a wee close to C's _Bool)
I don't particularly like just renaming it to bool_, for my part, but
certainly need to do something.
3) The 'bypassrls' boolean in AlterUser() is queried strangely. There are two
complaints:
I assume you mean AlterRole() above?
boolbypassrls = -1;
it's a boolean.
else if (authform-rolbypassrls || bypassrls = 0)
{
if (!superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg(must be superuser to change bypassrls
attribute)));
}
...
if (bypassrls = 0)
{
new_record[Anum_pg_authid_rolbypassrls - 1] =
BoolGetDatum(bypassrls 0);
new_record_repl[Anum_pg_authid_rolbypassrls - 1] = true;
}
if it's a boolean, that's always true.
It's not entirely clear to me why this was written that way. Stephen?
I specifically recall fixing a similar issue in this area but clearly
didn't realize that it was a bool when, as I recall, I was changing it
to match what we do for all of the other role attributes. In those
other cases the value is an int, not a bool though. Changing it to an
int here should make it line up with the rest of AlterRole().
I'll do that and add regression tests for it and any others which don't
get tested. My thinking on the test is to independently change each
value and then poll for all role attributes set and verify that the only
change made was the change expected.
3) vacuumdb.c stores -1's in a bool (result) in vacuum_one_database().
= convert to an int
Perhaps I'm missing something, but it appears to only ever be set to 0
or -1, so wouldn't it make sense to just correct it to use bool properly
instead?
As for what's happening, it appears that we'll happily continue to go
through all of the databases even on an error condition, right? I seem
to recall seeing that happen and being a bit surprised at it, but wasn't
in a position at the time to debug it.
4) _tableInfo-relreplident is a bool, should be a char
This results in pg_dump always failing to dump out the necessary
ALTER TABLE ONLY ... REPLICA IDENTITY ... when it's actually required,
right? Is that something we can add a regression test to cover which
will be exercised through the pg_upgrade path?
Thanks!
Stephen
signature.asc
Description: Digital signature
Re: [HACKERS] Warnings around booleans
* Andres Freund (and...@anarazel.de) wrote: On 2015-08-12 08:16:09 -0400, Stephen Frost wrote: 1) gin stores/queries some bools as GinTernaryValue. Part of this is easy to fix, just adjust GinScanKeyData-entryRes to be a GinTernaryValue (it's actually is compared against MAYBE). What I find slightly worrysome is that in gin_tsquery_consistent() checkcondition_gin (returning GinTernaryValue) is passed as a callback that's expected to return bool. And the field checkcondition_gin is returning (GinChkVal-check[i]) actually is a ternary. Is there a potential corruption issue from that..? I honestly don't understand the gin code well enough to answer that. Yeah, neither do I, so I've added Heikki. Heikki, any idea as to the impact of this? 2) help_config.c has a member named 'bool' in a local struct. That doesn't work well if bool actually is a macro. Which mean that whole #ifndef bool patch in c.h wasn't exercised since 2003 when help_config.c was created... = rename field to bool_ (or _bool but that looks a wee close to C's _Bool) I don't particularly like just renaming it to bool_, for my part, but certainly need to do something. There's alread a _enum in the struct, so that doesn't bother my much. Fair enough. 3) The 'bypassrls' boolean in AlterUser() is queried strangely. There are two complaints: [...] I specifically recall fixing a similar issue in this area but clearly didn't realize that it was a bool when, as I recall, I was changing it to match what we do for all of the other role attributes. In those other cases the value is an int, not a bool though. Changing it to an int here should make it line up with the rest of AlterRole(). I find that a somewhat ugly coding pattern, but since the rest of the function is written that way... Agreed. My role attributes patch had actually changed how that all was done, but once the discussion moved on to default roles instead of a bunch of additional role attributes, it felt like it would be unnecessary code churn to change that function. Perhaps we should do so anyway though. I'll do that and add regression tests for it and any others which don't get tested. My thinking on the test is to independently change each value and then poll for all role attributes set and verify that the only change made was the change expected. Do that if you like, but what I really think we should have is a test that tries to bypass rls and fails, then the user gets changes and it succeeds, and then it gets disabled and fails again. This really seems test-worthy behaviour to me. We certainly do have tests around BYPASSRLS, but not one which changes an independent role attribute and then re-tests. I'm fine with adding that test (or other tests, in general), but that won't help when another role attribute is added and someone makes a similar mistake, which I believe the test I'm thinking of will. Even if I'm unable to make that work though, someone grep'ing through our tests for places to add their new role attribute would likely catch a test that's checking all of the other ones while they might not consider what's done for just one attribute to be a generally applicable case. 4) _tableInfo-relreplident is a bool, should be a char This results in pg_dump always failing to dump out the necessary ALTER TABLE ONLY ... REPLICA IDENTITY ... when it's actually required, right? Is that something we can add a regression test to cover which will be exercised through the pg_upgrade path? With our current boolean definition this doesn't fail at all. All the values fall into both char and unsigned char range. WRT tests: It'd probably be a good idea to not drop the tables at the end of replica_identity.sql. Agreed. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
On 2015/08/12 7:21, Robert Haas wrote:
On Fri, Aug 7, 2015 at 3:37 AM, Kouhei Kaigai kai...@ak.jp.nec.com wrote:
I could have a discussion with Fujita-san about this topic.
Also, let me share with the discussion towards entire solution.
The primitive reason of this problem is, Scan node with scanrelid==0
represents a relation join that can involve multiple relations, thus,
its TupleDesc of the records will not fit base relations, however,
ExecScanFetch() was not updated when scanrelid==0 gets supported.
FDW/CSP on behalf of the Scan node with scanrelid==0 are responsible
to generate records according to the fdw_/custom_scan_tlist that
reflects the definition of relation join, and only FDW/CSP know how
to combine these base relations.
In addition, host-side expressions (like Plan-qual) are initialized
to reference the records generated by FDW/CSP, so the least invasive
approach is to allow FDW/CSP to have own logic to recheck, I think.
Below is the structure of ExecScanFetch().
ExecScanFetch(ScanState *node,
ExecScanAccessMtd accessMtd,
ExecScanRecheckMtd recheckMtd)
{
EState *estate = node-ps.state;
if (estate-es_epqTuple != NULL)
{
/*
* We are inside an EvalPlanQual recheck. Return the test tuple if
* one is available, after rechecking any access-method-specific
* conditions.
*/
Index scanrelid = ((Scan *) node-ps.plan)-scanrelid;
Assert(scanrelid 0);
if (estate-es_epqTupleSet[scanrelid - 1])
{
TupleTableSlot *slot = node-ss_ScanTupleSlot;
:
return slot;
}
}
return (*accessMtd) (node);
}
When we are inside of EPQ, it fetches a tuple in es_epqTuple[] array and
checks its visibility (ForeignRecheck() always say 'yep, it is visible'),
then ExecScan() applies its qualifiers by ExecQual().
So, as long as FDW/CSP can return a record that satisfies the TupleDesc
of this relation, made by the tuples in es_epqTuple[] array, rest of the
code paths are common.
I have an idea to solve the problem.
It adds recheckMtd() call if scanrelid==0 just before the assertion above,
and add a callback of FDW on ForeignRecheck().
The role of this new callback is to set up the supplied TupleTableSlot
and check its visibility, but does not define how to do this.
It is arbitrarily by FDW driver, like invocation of alternative plan
consists of only built-in logic.
Invocation of alternative plan is one of the most feasible way to
implement EPQ logic on FDW, so I think FDW also needs a mechanism
that takes child path-nodes like custom_paths in CustomPath node.
Once a valid path node is linked to this list, createplan.c transform
them to relevant plan node, then FDW can initialize and invoke this
plan node during execution, like ForeignRecheck().
This design can solve another problem Fujita-san has also mentioned.
If scan qualifier is pushed-down to the remote query and its expression
node is saved in the private area of ForeignScan, the callback on
ForeignRecheck() can evaluate the qualifier by itself. (Note that only
FDW driver can know where and how expression node being pushed-down
is saved in the private area.)
In the summary, the following three enhancements are a straightforward
way to fix up the problem he reported.
1. Add a special path to call recheckMtd in ExecScanFetch if scanrelid==0
2. Add a callback of FDW in ForeignRecheck() - to construct a record
according to the fdw_scan_tlist definition and to evaluate its
visibility, or to evaluate qualifier pushed-down if base relation.
3. Add List *fdw_paths in ForeignPath like custom_paths of CustomPaths,
to construct plan nodes for EPQ evaluation.
I'm not an expert in this area, but this plan does not seem unreasonable to me.
IIRC the discussion with KaiGai-san, I think that that would work. I
think that that would be more suitable for CSPs, though. Correct me if
I'm wrong, KaiGai-san. In either case, I'm not sure that the idea of
transferring both processing to a single callback routine hooked in
ForeignRecheck is a good idea: (a) check to see if the test tuple for
each component foreign table satisfies the remote qual condition and (b)
check to see if those tuples satisfy the remote join condition. I think
that that would be too complicated, probably making the callback routine
bug-prone. So, I'd still propose that *the core* processes (a) and (b)
*separately*.
* As for (a), the core checks the remote qual condition as in [1].
* As for (b), the core executes an alternative subplan locally if inside
an EPQ recheck. The subplan is created as described in [2].
Attached is a WIP patch for that against 9.5
(fdw-eval-plan-qual-0.1.patch), which includes an updated version of the
patch in [1]. I haven't done anything about custom joins yet. Also, I
left the join pushdown API
Re: [HACKERS] Tab completion for CREATE SEQUENCE
On 08/04/2015 06:30 PM, Robert Haas wrote: On Mon, Aug 3, 2015 at 2:17 AM, Michael Paquier michael.paqu...@gmail.com wrote: On Tue, Jul 7, 2015 at 9:14 PM, Andres Freund and...@anarazel.de wrote: On 2015-06-19 06:41:19 +, Brendan Jurd wrote: I'm marking this Waiting on Author. Once the problems have been corrected, it should be ready for a committer. Vik, are you going to update the patch? Seeing no activity on this thread and as it would be a waste not to do it, I completed the patch as attached. The following things are done: - Fixed code indentation - Removal of RESTART, SET SCHEMA, OWNER TO, and RENAME TO in CREATE SEQUENCE - Added START WITH. - Added TEMP/TEMPORARY in the set of keywords tracked. I am switching at the same time this patch as Ready for committer. You didn't remove RESTART, so I did that. And this needed some more parentheses to make my compiler happy. Committed with those changes. Thank you, Brendan, Michael, and Robert for taking care of this while I was away. -- Vik Fearing +33 6 46 75 15 36 http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- 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] Macro nesting hell
Andres Freund and...@anarazel.de writes: On 2015-07-01 12:55:48 -0300, Alvaro Herrera wrote: Tom Lane wrote: I'm thinking we really ought to mount a campaign to replace some of these macros with inlined-if-possible functions. My guess is that changing a very small amount of them will do a large enough portion of the job. I think it'd be good to convert the macros in bufpage.h and bufmgr.h that either currently have multiple evaluation hazards, or have a AssertMacro() in them. The former for obvious reasons, the latter because that immediately makes them rather large (on and it implies multiple evaluation hazards anyway). That'd mean inlining PageSetPageSizeAndVersion(), PageGetSpecialSize(), PageGetSpecialPointer(), PageGetItem(), PageGetMaxOffsetNumber(), PageIsPrunable(), PageSetPrunable(), PageClearPrunable(), BufferIsValid(), BufferGetBlock(), BufferGetPageSize(). Sounds reasonable to me. If you do this, I'll see whether pademelon can be adjusted to build using the minimum macro expansion buffer size specified by the C standard. 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] count_nulls(VARIADIC any)
Hi,
I'd like to suggest $SUBJECT for inclusion in Postgres 9.6. I'm sure
everyone would've found it useful at some point in their lives, and the
fact that it can't be properly implemented in any language other than C
I think speaks for the fact that we as a project should provide it.
A quick and dirty proof of concept (patch attached):
=# select count_nulls(null::int, null::text, 17, 'bar');
count_nulls
-
2
(1 row)
Its natural habitat would be CHECK constraints, e.g:
CHECK (count_nulls(a,b,c) IN (0, 3))
Will finish this up for the next CF, unless someone wants to tell me how
stupid this idea is before that.
.m
*** a/src/backend/utils/adt/misc.c
--- b/src/backend/utils/adt/misc.c
***
*** 45,50
--- 45,119
/*
+ * count_nulls()
+ * Count the number of NULL input arguments
+ */
+ Datum
+ pg_count_nulls(PG_FUNCTION_ARGS)
+ {
+ int32 count = 0;
+ int i;
+
+ if (get_fn_expr_variadic(fcinfo-flinfo))
+ {
+ ArrayType *arr;
+ int ndims, nitems, *dims;
+ bits8 *bitmap;
+ int bitmask;
+
+ /* Should have just the one argument */
+ Assert(PG_NARGS() == 1);
+
+ /* count_nulls(VARIADIC NULL) is defined as NULL */
+ if (PG_ARGISNULL(0))
+ PG_RETURN_NULL();
+
+ /*
+* Non-null argument had better be an array. We assume that
any call
+* context that could let get_fn_expr_variadic return true will
have
+* checked that a VARIADIC-labeled parameter actually is an
array. So
+* it should be okay to just Assert that it's an array rather
than
+* doing a full-fledged error check.
+*/
+
Assert(OidIsValid(get_base_element_type(get_fn_expr_argtype(fcinfo-flinfo,
0;
+
+ /* OK, safe to fetch the array value */
+ arr = PG_GETARG_ARRAYTYPE_P(0);
+
+ ndims = ARR_NDIM(arr);
+ dims = ARR_DIMS(arr);
+ nitems = ArrayGetNItems(ndims, dims);
+ //if (nitems == 0)
+ // return PG_RETURN_INT32(0);
+
+ bitmap = ARR_NULLBITMAP(arr);
+ if (!bitmap)
+ PG_RETURN_INT32(0);
+
+ for (i = 0; i nitems; i++)
+ {
+ if ((*bitmap bitmask) == 0)
+ count++;
+
+ bitmask = 1;
+ if (bitmask == 0x100)
+ {
+ bitmap++;
+ bitmask = 1;
+ }
+ }
+ PG_RETURN_INT32(count);
+ }
+
+ for (i = 0; i PG_NARGS(); i++)
+ {
+ if (PG_ARGISNULL(i))
+ count++;
+ }
+ PG_RETURN_INT32(count);
+ }
+
+ /*
* current_database()
*Expose the current database to the user
*/
*** a/src/include/catalog/pg_proc.h
--- b/src/include/catalog/pg_proc.h
***
*** 2959,2964 DESCR(adjust time with time zone precision);
--- 2959,2965
DATA(insert OID = 2003 ( textanycat PGNSP PGUID 14 1 0 0 0 f f f f t f s
2 0 25 25 2776 _null_ _null_ _null_ _null_ _null_ select $1 ||
$2::pg_catalog.text _null_ _null_ _null_ ));
DATA(insert OID = 2004 ( anytextcat PGNSP PGUID 14 1 0 0 0 f f f f t f s
2 0 25 2776 25 _null_ _null_ _null_ _null_ _null_ select $1::pg_catalog.text
|| $2 _null_ _null_ _null_ ));
+ DATA(insert OID = 3308 ( count_nullsPGNSP PGUID 12 1 0 2276 0 f f f f f
f i 1 0 23 2276 {2276} {v} _null_ _null_ _null_pg_count_nulls _null_
_null_ _null_ ));
DATA(insert OID = 2005 ( bytealike PGNSP PGUID 12 1 0 0 0 f f f
f t f i 2 0 16 17 17 _null_ _null_ _null_ _null_ _null_ bytealike _null_
_null_ _null_ ));
DATA(insert OID = 2006 ( byteanlike PGNSP PGUID 12 1 0 0 0 f f f f t f i
2 0 16 17 17 _null_ _null_ _null_ _null_ _null_ byteanlike _null_ _null_
_null_ ));
*** a/src/include/utils/builtins.h
--- b/src/include/utils/builtins.h
***
*** 481,486 extern Datum pg_ls_dir(PG_FUNCTION_ARGS);
--- 481,487
extern Datum pg_ls_dir_1arg(PG_FUNCTION_ARGS);
/* misc.c */
+ extern Datum pg_count_nulls(PG_FUNCTION_ARGS);
extern Datum current_database(PG_FUNCTION_ARGS);
extern Datum current_query(PG_FUNCTION_ARGS);
extern Datum pg_cancel_backend(PG_FUNCTION_ARGS);
--
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] can't coax query planner into using all columns of a gist index
I've created a small dump of my database that recreates the problem. I hope that this will help recreate the problem. It is attached. I'd be happy to hear if there is an easier way of doing this. To rebuild the database: - create a database - run from the commandline `$ psql database-name 1000genomes-schema.sql` - run this within a psql REPL ` #\copy public.qcregions FROM '/tmp/1000genomes-qcregions.tsv' DELIMITER ' ' CSV;` (where the delimiter is a tab) - similarly run this within a psql REPL, `#\copy public.vcf FROM '/tmp/1000genomes-vcf.tsv' DELIMITER ' ' CSV;` To see that the GIST index is not being hit, try running the following query: EXPLAIN SELECT * FROM vcf WHERE EXISTS (SELECT region FROM qcregions WHERE qcregions.chr = vcf.chr AND vcf.pos @ qcregions.region); The actual query I am trying to run is: EXPLAIN SELECT * FROM vcf WHERE EXISTS (SELECT region FROM qcregions WHERE qcregions.chr = vcf.chr AND qcregions.type = 'include' AND vcf.pos @ qcregions.region); Let me know what else I can try, Gideon. On Wed, Aug 12, 2015 at 11:07 AM Gideon Dresdner gide...@gmail.com wrote: What's a good way for me to create a self-contained test case. AFAIU the only way to make these test cases more self-contained would be to inline the second table and its index. How do you create an index to an inlined table of values? Or perhaps I could send over a dump of a subset of the data? Yes, I am fairly sure that I am running 9.4.4: $ psql --version psql (PostgreSQL) 9.4.4 # select version(); version --- PostgreSQL 9.4.4 on x86_64-unknown-linux-gnu, compiled by gcc (GCC) 5.1.0, 64-bit (1 row) Thanks for the help, Gideon. On Tue, Aug 11, 2015 at 10:23 PM Tom Lane t...@sss.pgh.pa.us wrote: Gideon Dresdner gide...@gmail.com writes: I had a discussion on IRC today with RhodiumToad regarding optimizing a specific query. We didn't manage to figure out how to get postgres to hit a GIST index. FWIW, I couldn't reproduce the described behavior. Can you provide a self-contained test case? Are you sure your server is 9.4.4? regards, tom lane 1000genomes-dump.tar.gz Description: application/gzip -- 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] count_nulls(VARIADIC any)
On Wed, Aug 12, 2015 at 6:18 PM, Marko Tiikkaja ma...@joh.to wrote: Will finish this up for the next CF, unless someone wants to tell me how stupid this idea is before that. I'm kind of puzzled what kind of schema would need this. -- greg -- 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] count_nulls(VARIADIC any)
Greg Stark wrote: On Wed, Aug 12, 2015 at 6:18 PM, Marko Tiikkaja ma...@joh.to wrote: Will finish this up for the next CF, unless someone wants to tell me how stupid this idea is before that. I'm kind of puzzled what kind of schema would need this. I've seen cases where you want some entity to be of either of some number of types. In those cases you have nullable FKs in separate columns, and only one of them can be non null. The name count_nulls() suggest an aggregate function to me, though. -- Álvaro Herrerahttp://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] Test code is worth the space
On Wed, Aug 12, 2015 at 10:46 AM, Greg Stark st...@mit.edu wrote: The only time I've seen pushback against tests is when the test author made valiant efforts to test every codepath and the expected output embeds the precise behaviour of the current code as correct. Even when patches have extensive tests I don't recall seeing much pushback (though I've been having trouble keeping up with the list in recent months) if the tests are written in a way that they will only fail if there's a bug, even if behaviour changes in unrelated ways. Really? I think Noah's description of how less testing is in effect incentivized by committers is totally accurate. No patch author is going to dig their heals in over the objections of a committer when the complaint is about brevity of tests. This resistance to adding tests seems quite short sighted to me, especially when the concern is about queries that will each typically take less than 1ms to execute. Like Noah, I think that it would be very helpful to simply be more inclusive of additional tests that don't increase test coverage by as much as each query in a minimal subset. I am not at all convinced by arguments about the cost of maintaining tests when a simple behavioral change occurs. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Raising our compiler requirements for 9.6
On 2015-08-12 13:00:25 -0400, Robert Haas wrote: On Tue, Aug 11, 2015 at 10:34 PM, Noah Misch n...@leadboat.com wrote: In my opinion this drastically increases readability and thus should be applied. Will do so sometime tomorrow unless there's protest. -1 to introducing more inline functions before committable code replaces what you've already pushed for this thread. This appears to be intended as an insult, but maybe I'm misreading it. I read it as one. I am not thrilled with the rate at which this stuff is getting whacked around. Less-significant changes have been debated for far longer, and I really doubt that the rate at which Andres is committing changes in this area is healthy. There was one feature patch committed about the actual topic so far (de6fd1c), and then some fixes to handle the portability fallout and a admittedly stypid typo. We've debated the inline thing for years now and seem to have a come to a conclusion about a month ago (http://archives.postgresql.org/message-id/27127.1435791908%40sss.pgh.pa.us). You then brought up the thread again (ca+tgmoaavfx1kvz5wbkvi1o6ozrxzf0micsttao7gguv5a4...@mail.gmail.com) sometimes after that I started to work on a patch. Maybe I should have waited bit more to commit the initial, rather mechanical, conversion to inlines patch, although I don't see what that'd really have bought us: This kind of patch (tinkering with autoconf, portability and the like) normally triggers just about no feedback until it has caused problems. The buildfarm exists to catch such portability problems. The issue we're actually arguing about (lockdefs split) was indeed necessitated by a portability issue (30786.1438831...@sss.pgh.pa.us). One that actually has existed at least since atomics went in - it just happens that most (but not all, c.f. a9baeb361d and 7507b19) compilers that support inlines don't emit references to symbols referenced in a unused inline function. Since nobody noticed that issue in the 1+ years atomics was learning to walk on list, and not in the 8 months since it was committed, it's quite obviously not obvious. WRT the lockdefs.h split. It wasn't my idea (IIRC Alvaro's; I wanted to do something closes to what Noah suggested before Tom objected to lock.h in FRONTEND programs!), and I did wait for a day, while the buildfarm was red!, after posting it. At least two committers did comment on the approach before I pushed it. We've seen far more hot-needled patches to fix the buildfarm in topics with less portability risks. I could have left the buildfarm red for longer, but that might have hidden more severe portability problems. It's not like I committed that patch after somebody had suggested another way: Noah only commented after I had already pushed the split. The only actual separate patch since then (fastgetattr as inline function) was posted 2015-08-05 and I yesterday suggested to push it today. And it's just replacing two existing macros by inline functions. Imo there are far more complex patches regularly get committed by various people, with less discussion and review. There's afaik nothing broken due to either the inline patch or the lockdefs split right now. There's one portability bandaid, judged to be ugly by some, that we're discussing right now, and I'm happy to rejigger it if the various people with contradicting opinions can come to an agreement. -- 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 pending clean up is not interruptable
On Tue, Aug 11, 2015 at 5:27 PM, Tom Lane t...@sss.pgh.pa.us wrote: Andres Freund and...@anarazel.de writes: On 2015-08-11 15:07:15 -0700, Jeff Janes wrote: The attached patch adds an else branch to call CHECK_FOR_INTERRUPTS(). But I think we could instead just call vacuum_delay_point unconditionally. It calls CHECK_FOR_INTERRUPTS(), and if not in a throttled vacuum it does nothing else. (That is how ANALYZE handles it.) Hm, I find that not exactly pretty. I'd rather just add an unconditional CHECK_FOR_INTERRUPTS to the function. CHECK_FOR_INTERRUPTS is very cheap. But I tend to agree that you should be using vacuum_delay_point. Attached patch does it that way. There was also a free-standing CHECK_FOR_INTERRUPTS() which had no reason that I could see not be a vacuum_delay_point, so I changed that one as well. With this patch, ctrl-C and 'pg_ctl stop -mf' both behave nicely. Cheers, Jeff gin_pendinglist_interrupt.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] [COMMITTERS] pgsql: Close some holes in BRIN page assignment
Alvaro Herrera wrote: Close some holes in BRIN page assignment buildfarm evidently didn't like this one :-( -- Álvaro Herrerahttp://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] count_nulls(VARIADIC any)
Hi 2015-08-12 19:18 GMT+02:00 Marko Tiikkaja ma...@joh.to: Hi, I'd like to suggest $SUBJECT for inclusion in Postgres 9.6. I'm sure everyone would've found it useful at some point in their lives, and the fact that it can't be properly implemented in any language other than C I think speaks for the fact that we as a project should provide it. A quick and dirty proof of concept (patch attached): =# select count_nulls(null::int, null::text, 17, 'bar'); count_nulls - 2 (1 row) Its natural habitat would be CHECK constraints, e.g: CHECK (count_nulls(a,b,c) IN (0, 3)) Will finish this up for the next CF, unless someone wants to tell me how stupid this idea is before that. It is not bad idea +1 Pavel .m -- 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] Test code is worth the space
On Wed, Aug 12, 2015 at 3:10 AM, Noah Misch n...@leadboat.com wrote: Committers press authors to delete tests more often than we press them to resubmit with more tests. No wonder so many patches have insufficient tests; we treat those patches more favorably, on average. I have no objective principles for determining whether a test is pointlessly redundant, but I think the principles should become roughly 10x more permissive than the (unspecified) ones we've been using. I would suggest the metric should be if this test fails is it more likely to be noise due to an intentional change in behaviour or more likely to be a bug? The only time I've seen pushback against tests is when the test author made valiant efforts to test every codepath and the expected output embeds the precise behaviour of the current code as correct. Even when patches have extensive tests I don't recall seeing much pushback (though I've been having trouble keeping up with the list in recent months) if the tests are written in a way that they will only fail if there's a bug, even if behaviour changes in unrelated ways. -- greg -- 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 oversight converting buf_id to Buffer
All look good to me, Thank you, Qingqing On Wed, Aug 12, 2015 at 8:37 AM, Andres Freund and...@anarazel.de wrote: Hi, On 2015-08-11 01:15:37 +0200, Andres Freund wrote: I'm too tired right now to look at this, but it generally looked sane. Pushed your fix to master and 9.5, with two very minor changes: 1) I moved the BufferDescriptorGetBuffer() call in PinBuffer_Locked() to after the spinlock release. It's rather minor, but there seems little reason to do it before except the assert, which isn't compiled in production. 2) I removed the two asserts you added. They essentially asserted that i + 1 == i + 1. Thanks again for the catch and patch. 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
[HACKERS] Re: [COMMITTERS] Using contrib modules in check (Re: pgsql: Fix BRIN to use SnapshotAny during summarization)
Tom Lane wrote:
However, we did learn something valuable from the fact that all the
-DCLOBBER_CACHE_ALWAYS critters failed on it: per my earlier message,
brin_page_items() is unsafe against a relcache flush on the index.
I'll put that on the 9.5 open items list.
(If I were tasked with fixing it, I'd be tempted to rewrite it to do
all the work in one call and return a tuplestore; the alternative
seems to be to try to keep the index open across multiple calls,
which would be a mess.)
Here's a patch doing that.
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training Services
commit 41eba17795e9cf9b0c23e05def5d33b0dbdd4807[m
Author: Alvaro Herrera alvhe...@alvh.no-ip.org
AuthorDate: Wed Aug 12 15:41:15 2015 -0300
CommitDate: Wed Aug 12 15:41:15 2015 -0300
fix brin_page_items to use a tuplestore
diff --git a/contrib/pageinspect/brinfuncs.c b/contrib/pageinspect/brinfuncs.c
index 7adcfa8..f695b18 100644
--- a/contrib/pageinspect/brinfuncs.c
+++ b/contrib/pageinspect/brinfuncs.c
@@ -37,18 +37,6 @@ typedef struct brin_column_state
FmgrInfo outputFn[FLEXIBLE_ARRAY_MEMBER];
} brin_column_state;
-typedef struct brin_page_state
-{
- BrinDesc *bdesc;
- Page page;
- OffsetNumber offset;
- bool unusedItem;
- bool done;
- AttrNumber attno;
- BrinMemTuple *dtup;
- brin_column_state *columns[FLEXIBLE_ARRAY_MEMBER];
-} brin_page_state;
-
static Page verify_brin_page(bytea *raw_page, uint16 type,
const char *strtype);
@@ -119,89 +107,90 @@ verify_brin_page(bytea *raw_page, uint16 type, const char *strtype)
Datum
brin_page_items(PG_FUNCTION_ARGS)
{
- brin_page_state *state;
- FuncCallContext *fctx;
+ bytea *raw_page = PG_GETARG_BYTEA_P(0);
+ Oid indexRelid = PG_GETARG_OID(1);
+ ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo-resultinfo;
+ TupleDesc tupdesc;
+ MemoryContext oldcontext;
+ Tuplestorestate *tupstore;
+ Relation indexRel;
+ Page page;
+ OffsetNumber offset;
+ AttrNumber attno;
+ bool unusedItem;
+ BrinDesc *bdesc;
+ BrinMemTuple *dtup;
+ brin_column_state **columns;
if (!superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
(errmsg(must be superuser to use raw page functions;
- if (SRF_IS_FIRSTCALL())
+ /* check to see if caller supports us returning a tuplestore */
+ if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo))
+ ereport(ERROR,
+(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg(set-valued function called in context that cannot accept a set)));
+ if (!(rsinfo-allowedModes SFRM_Materialize) ||
+ rsinfo-expectedDesc == NULL)
+ ereport(ERROR,
+(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg(materialize mode required, but it is not allowed in this context)));
+
+ /* Build a tuple descriptor for our result type */
+ if (get_call_result_type(fcinfo, NULL, tupdesc) != TYPEFUNC_COMPOSITE)
+ elog(ERROR, return type must be a row type);
+
+ /* Build tuplestore to hold the result rows */
+ oldcontext = MemoryContextSwitchTo(rsinfo-econtext-ecxt_per_query_memory);
+
+ tupstore = tuplestore_begin_heap(true, false, work_mem);
+ rsinfo-returnMode = SFRM_Materialize;
+ rsinfo-setResult = tupstore;
+ rsinfo-setDesc = tupdesc;
+
+ MemoryContextSwitchTo(oldcontext);
+
+ indexRel = index_open(indexRelid, AccessShareLock);
+
+ /* minimally verify the page we got */
+ page = verify_brin_page(raw_page, BRIN_PAGETYPE_REGULAR, regular);
+
+ bdesc = brin_build_desc(indexRel);
+ offset = FirstOffsetNumber;
+ unusedItem = false;
+ dtup = NULL;
+
+ /*
+ * Initialize output functions for all indexed datatypes; simplifies
+ * calling them later.
+ */
+ columns = palloc(sizeof(brin_column_state *) * RelationGetDescr(indexRel)-natts);
+ for (attno = 1; attno = bdesc-bd_tupdesc-natts; attno++)
{
- bytea *raw_page = PG_GETARG_BYTEA_P(0);
- Oid indexRelid = PG_GETARG_OID(1);
- Page page;
- TupleDesc tupdesc;
- MemoryContext mctx;
- Relation indexRel;
- AttrNumber attno;
+ Oid output;
+ bool isVarlena;
+ BrinOpcInfo *opcinfo;
+ int i;
+ brin_column_state *column;
- /* minimally verify the page we got */
- page = verify_brin_page(raw_page, BRIN_PAGETYPE_REGULAR, regular);
+ opcinfo = bdesc-bd_info[attno - 1];
+ column = palloc(offsetof(brin_column_state, outputFn) +
+ sizeof(FmgrInfo) * opcinfo-oi_nstored);
- /* create a function context for cross-call persistence */
- fctx = SRF_FIRSTCALL_INIT();
-
- /* switch to memory context appropriate for multiple function calls */
- mctx = MemoryContextSwitchTo(fctx-multi_call_memory_ctx);
-
- /* Build a tuple descriptor for our result type */
- if (get_call_result_type(fcinfo, NULL, tupdesc) != TYPEFUNC_COMPOSITE)
- elog(ERROR, return type must be a row type);
-
- indexRel = index_open(indexRelid, AccessShareLock);
-
- state = palloc(offsetof(brin_page_state, columns) +
- sizeof(brin_column_state) * RelationGetDescr(indexRel)-natts);
-
- state-bdesc =
Re: [HACKERS] count_nulls(VARIADIC any)
On 2015-08-12 7:23 PM, Greg Stark wrote: On Wed, Aug 12, 2015 at 6:18 PM, Marko Tiikkaja ma...@joh.to wrote: Will finish this up for the next CF, unless someone wants to tell me how stupid this idea is before that. I'm kind of puzzled what kind of schema would need this. The first example I could find from our schema was specifying the URL for a Remote Procedure Call. You can either specify a single request URI, or you can specify the pieces: protocol, host, port, path. So the constraints look roughly like this: CHECK ((fulluri IS NULL) (protocol IS NULL)), CHECK ((protocol IS NULL) = ALL(ARRAY[host IS NULL, port IS NULL, path IS NULL])) Obviously the second one would be much prettier with count_nulls(). The other example is an OOP inheritance-like schema where an object could be one of any X number of types. You could write that: CHECK ((a IS NULL)::int + (b IS NULL)::int + (c IS NULL)::int) = 1) or just: CHECK (count_nulls(a,b,c) = 1) The first example could be redesigned with three tables, but that seems like a cure worse than the disease. .m -- 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] count_nulls(VARIADIC any)
On Wed, Aug 12, 2015 at 10:30 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: The name count_nulls() suggest an aggregate function to me, though. I thought the same. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] count_nulls(VARIADIC any)
On 2015-08-12 7:35 PM, Pavel Stehule wrote: maybe nulls_count ? we have regr_count already But that's an aggregate as well.. .m -- 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] count_nulls(VARIADIC any)
2015-08-12 19:37 GMT+02:00 Marko Tiikkaja ma...@joh.to: On 2015-08-12 7:35 PM, Pavel Stehule wrote: maybe nulls_count ? we have regr_count already But that's an aggregate as well.. my mistake Pavel .m
Re: [HACKERS] count_nulls(VARIADIC any)
2015-08-12 19:32 GMT+02:00 Peter Geoghegan p...@heroku.com: On Wed, Aug 12, 2015 at 10:30 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: The name count_nulls() suggest an aggregate function to me, though. I thought the same. maybe nulls_count ? we have regr_count already Regards Pavel -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Test code is worth the space
One trouble I face when adding tests is that sometimes they require hooks in the code, to test for race conditions. In BRIN I cannot test some code paths without resorting to adding breakpoints in GDB, for instance. If there's no support for such in the core code, it's essentially impossible to add meaningful test for certain code paths. I toyed with the notion of adding hook points (such that a test module can put the backend to sleep until the other backend acknowledges hitting the race condition window); I decided not to because it seems a more general discussion is necessary first about the desirability of such. As I recall we needed this in SKIP LOCKED and of course for multixacts also. I agree with the general idea that it's worthwhile to add lots more tests than we currently have, both for the current code and for future patches. Surely we don't need to reach the point where every single nuance of every single user interface is verified to the point that tests are such a large burden to maintain as is being suggested. -- Álvaro Herrerahttp://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] Warnings around booleans
Hi Michael,
I'm currently investigating some of our code cleanliness issues around
booleans. Turns out that ecpg fails if C99's _Bool is used as bool
instead of typedef char bool.
Playing around a bit lead to to find that this is caused by a wrong type
declaration in two places. 'isarray' is declared as bool instead of enum
ARRAY_TYPE in two places. This appears to be an oversight, perhaps
caused by the boolean sounding name.
Does this look right to you? If so, will you apply it?
- Andres
From 8335ad3b9b96964f0f4eac7c16fb4016c6370acb Mon Sep 17 00:00:00 2001
From: Andres Freund and...@anarazel.de
Date: Wed, 12 Aug 2015 21:44:49 +0200
Subject: [PATCH] ecpg: Use ARRAY_TYPE instead of a bool to store the array
type.
Using a bool isn't just somewhat ugly, but actually fails badly if bool
is mapped to C99's _Bool. This appears to be caused by an oversight
instead of a conscious decision.
---
src/interfaces/ecpg/ecpglib/execute.c | 2 +-
src/interfaces/ecpg/ecpglib/extern.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c
index 9a56a5c..9e40f41 100644
--- a/src/interfaces/ecpg/ecpglib/execute.c
+++ b/src/interfaces/ecpg/ecpglib/execute.c
@@ -146,7 +146,7 @@ next_insert(char *text, int pos, bool questionmarks)
}
static bool
-ecpg_type_infocache_push(struct ECPGtype_information_cache ** cache, int oid, bool isarray, int lineno)
+ecpg_type_infocache_push(struct ECPGtype_information_cache ** cache, int oid, enum ARRAY_TYPE isarray, int lineno)
{
struct ECPGtype_information_cache *new_entry
= (struct ECPGtype_information_cache *) ecpg_alloc(sizeof(struct ECPGtype_information_cache), lineno);
diff --git a/src/interfaces/ecpg/ecpglib/extern.h b/src/interfaces/ecpg/ecpglib/extern.h
index 1fa21fd..263e001 100644
--- a/src/interfaces/ecpg/ecpglib/extern.h
+++ b/src/interfaces/ecpg/ecpglib/extern.h
@@ -44,7 +44,7 @@ struct ECPGtype_information_cache
{
struct ECPGtype_information_cache *next;
int oid;
- bool isarray;
+ enum ARRAY_TYPE isarray;
};
/* structure to store one statement */
--
2.3.0.149.gf3f4077.dirty
--
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: SCRAM authentication
On 8/12/15 12:19 PM, Robert Haas wrote: On Wed, Aug 12, 2015 at 10:50 AM, Peter Eisentraut pete...@gmx.net wrote: I understand this idea, but I think it's not practical for many uses. There is no way to find out, on the server, whether all current clients would support a switch to SCRAM. Let alone all not-current clients. The only way to do such a switch would be to do the switch and then check for connection failures in the log, which is not good. Well, number one, I don't think we log anything currently that would let you figure that out; We have options to log details about authenticated connections. This kind of thing could easily be added there. and number two, I'm not sure I believe that's the only way to make sure you've updated your clients. Well, if there is another one, I haven't heard about it so far. (Now that I think about this, to counter my point, this is very similar to the switch from crypt to md5. You couldn't turn that on until you were sure that all clients would support it. I don't remember the experience from back then, though.) Maybe we should try to find out how that played out. It could inform the current discussion. I think in those days, installations weren't very long-lived. md5 was introduced in version 7.2, which is also the first version with lazy vacuum. Another thing we might want to try to find out is: if we add SCRAM authentication to 9.6, how committed are drivers authors to adding that support to their drivers? If we poll the maintainers of the drivers for Perl, Python, Ruby, Node.JS, Java, ODBC, etc. and involve them in this conversation, we might learn useful things. This is a big change we're talking about, and it's only to work (regardless of the details) if the driver authors are on board. We haven't, AFAIK, talked to them about this at all. I'm not concerned about whether drivers are willing to support this. These sort of things have usually worked out in the past. But just because a driver supports a new authentication method, that doesn't mean everyone can or will upgrade to it right away. -- 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: SCRAM authentication
All, * Peter Eisentraut (pete...@gmx.net) wrote: On 8/12/15 12:19 PM, Robert Haas wrote: On Wed, Aug 12, 2015 at 10:50 AM, Peter Eisentraut pete...@gmx.net wrote: I understand this idea, but I think it's not practical for many uses. There is no way to find out, on the server, whether all current clients would support a switch to SCRAM. Let alone all not-current clients. The only way to do such a switch would be to do the switch and then check for connection failures in the log, which is not good. Well, number one, I don't think we log anything currently that would let you figure that out; We have options to log details about authenticated connections. This kind of thing could easily be added there. and number two, I'm not sure I believe that's the only way to make sure you've updated your clients. Well, if there is another one, I haven't heard about it so far. Agreed. (Now that I think about this, to counter my point, this is very similar to the switch from crypt to md5. You couldn't turn that on until you were sure that all clients would support it. I don't remember the experience from back then, though.) Maybe we should try to find out how that played out. It could inform the current discussion. I think in those days, installations weren't very long-lived. md5 was introduced in version 7.2, which is also the first version with lazy vacuum. I don't believe that's any basis for comparison given that it was well over 10 years ago that we released 7.2. Another thing we might want to try to find out is: if we add SCRAM authentication to 9.6, how committed are drivers authors to adding that support to their drivers? If we poll the maintainers of the drivers for Perl, Python, Ruby, Node.JS, Java, ODBC, etc. and involve them in this conversation, we might learn useful things. This is a big change we're talking about, and it's only to work (regardless of the details) if the driver authors are on board. We haven't, AFAIK, talked to them about this at all. I'm not concerned about whether drivers are willing to support this. These sort of things have usually worked out in the past. But just because a driver supports a new authentication method, that doesn't mean everyone can or will upgrade to it right away. I certainly want driver authors to support this and would be willing to work with them to make it happen. Discussing the change with them certainly makes sense, in general. As for the notion of dropping md5 from 9.6 or even forcing it to be one-or-the-other on a per-role basis, I'm surprised we're even discussing it. We've already got people who aren't upgrading to newer, supported, versions of PG because the application they have won't work for some reason, this is creating a much harder upgrade path for those users, and I don't believe that the security concern holds any water compared to that issue. For starters, if the attacker has access to the md5 hash in the DB, the game is over already, full stop, when it comes to accessing the database as that user. We won't fix that until everyone's on SCRAM, so we should be making it as easy as possible to get there. I don't believe having the SCRAM password verifier in the DB will seriously improve an attacker's chances of getting the plaintext password simply because our md5 salt completely sucks (no one serious about security would use it) and md5 simply doesn't stand up to attacks very well these days. I'm all for having an eventual flag day where we ship a release that doesn't support md5 on the server side as an authentication mechanism and pg_upgrade has a check which prevents upgrades if there are any md5 entries left, but that's at least two and probably three releases out. What we need to be doing now is getting that ball rolling or we're never going to actually get there. Thanks! Stephen signature.asc Description: Digital signature
