Re: [HACKERS] Parallel Seq Scan
On 1/11/15 3:57 PM, Robert Haas wrote: On Sun, Jan 11, 2015 at 5:27 AM, Stephen Frost sfr...@snowman.net wrote: * Robert Haas (robertmh...@gmail.com) wrote: On Thu, Jan 8, 2015 at 2:46 PM, Stephen Frost sfr...@snowman.net wrote: Yeah, if we come up with a plan for X workers and end up not being able to spawn that many then I could see that being worth a warning or notice or something. Not sure what EXPLAIN has to do anything with it.. That seems mighty odd to me. If there are 8 background worker processes available, and you allow each session to use at most 4, then when there are 2 sessions trying to do parallelism at the same time, they might not all get their workers. Emitting a notice for that seems like it would be awfully chatty. Yeah, agreed, it could get quite noisy. Did you have another thought for how to address the concern raised? Specifically, that you might not get as many workers as you thought you would? I'm not sure why that's a condition in need of special reporting. The case raised before (that I think is valid) is: what if you have a query that is massively parallel. You expect it to get 60 cores on the server and take 10 minutes. Instead it gets 10 and takes an hour (or worse, 1 and takes 10 hours). Maybe it's not worth dealing with that in the first version, but I expect it will come up very quickly. We better make sure we're not painting ourselves in a corner. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Async execution of postgres_fdw.
On Fri, Jan 9, 2015 at 2:00 PM, Kyotaro HORIGUCHI
horiguchi.kyot...@lab.ntt.co.jp wrote:
Hello, thank you for the comment.
This is the second version of the patch.
- Refactored to make the code simpler and clearer.
- Added comment about logic outline and struct members.
- Removed trailig white spaces..
- No additional test yet.
==
warning: 3 lines add whitespace errors.
Whoops. Fixed.
2. The patches compile cleanly.
3. The regression is clean, even in contrib/postgres_fdw and
contrib/file_fdw
Tests
---
We need tests to make sure that the logic remains intact even after
further
changes in this area. Couple of tests which require multiple foreign
scans
within the same query fetching rows more than fetch size (100) would be
required. Also, some DMLs, which involve multiple foreign scans would
test
the sanity when UPDATE/DELETE interleave such scans. By multiple foreign
scans I mean both multiple scans on a single foreign server and multiple
scans spread across multiple foreign servers.
Additional tests indeed might be needed. Some of the test related
to this patch are implicitly done in the present regression
tests. But no explicit ones.
fetch_size is currently a bare constant so I think it is not so
necessary to test for other fetch sizes. Even if different size
will potentially cause a problem, it will be found when the
different number is actually applied.
On the current design, async scan is started only on the first
scan on the connection, and if the next scan or modify claims the
same connection, the async state is immediately finished and
behaves as the same as the unpatched version. But since
asynchronous/parallel scan is introduced in any form, such kind
of test seems to be needed.
multi-server tests are not done also in the unpatched version but
there's no difference between multiple foregn servers on the same
remote server and them distributed on multiple remotes. The async
scan of this patch works only on the same foreign server so there
seems to be no need such kind of test. Do you have any specific
concern about this?
After all, I will add some explict tests for async-canceling in
the next patch.
Code
---
Because previous conn is now replaced by conn-pgconn, the double
indirection makes the code a bit ugly and prone to segfaults (conn being
NULL or invalid pointer). Can we minimize such code or wrap it under a
macro?
Agreed. It was annoyance also for me. I've done the following
things to encapsulate PgFdwConn to some extent in the second
version of this patch. They are described below.
Looks better.
We need some comments about the structure definition of PgFdwConn and its
members explaining the purpose of this structure and its members.
Thank you for pointing that. I forgot that. I added simple
comments there.
Same is the case with enum PgFdwConnState. In fact, the state diagram of
a
connection has become more complicated with the async connections, so it
might be better to explain that state diagram at one place in the code
(through comments). The definition of the enum might be a good place to
do
that.
I added a comment describing the and logic and meaning of the
statesjust above the enum declaration.
This needs to be clarified further. But that can wait till we finalise the
approach and the patch. Esp. comment below is confusing
1487 + * PGFDW_CONN_SYNC_RUNNING is rather an internal state in
1488 + * fetch_more_data(). It indicates that the function shouldn't send
the next
1489 + * fetch requst after getting the result.
I couldn't get the meaning of the second sentence, esp. it's connection
with synchronous-ness.
Otherwise, the logic of connection maintenance is spread at multiple
places and is difficult to understand by looking at the code.
In function GetConnection(), at line
elog(DEBUG3, new postgres_fdw connection %p for server \%s\,
-entry-conn, server-servername);
+entry-conn-pgconn, server-servername);
Thank you, I replaced conn's in this form with PFC_PGCONN(conn).
This looks better.
entry-conn-pgconn may not necessarily be a new connection and may be
NULL
(as the next line check it for being NULL). So, I think this line should
be
moved within the following if block after pgconn has been initialised
with
the new connection.
+ if (entry-conn-pgconn == NULL)
+ {
+ entry-conn-pgconn = connect_pg_server(server, user);
+ entry-conn-nscans = 0;
+ entry-conn-async_state = PGFDW_CONN_IDLE;
+ entry-conn-async_scan = NULL;
+ }
The if condition if (entry-conn == NULL) in GetConnection(), used to
track
whether there is a PGConn active for the given entry, now it tracks
whether
it has PgFdwConn for the same.
After some soncideration, I decided to make PgFdwConn to be
handled more similarly to PGconn. This patch has shrunk as a
result and bacame
[HACKERS] ereport bug
Hello, postgresmen! I found incorrect execution of ereport() macro. If we pass into ereport() function 2 or more arguments, the macro errcontext does not correct execute. So, ereport() call stack is: errstarterrcontext_msgset_errcontext_domainerrmsgerrfinishpg_unreachable This bug causes that error messages (for example, in PL/TCL) are not localized.Solutions:- Wrap all errcontext() macro in brackets, that is errcontext("error message %s", "end message") - (errcontext("error message %s", "end message"))- Rewrite this macro- ??? I am attaching to this letter a test case that shows the behavior errcontext() macro and the way to fix it.I am using postgresql 9.4 and test it on gcc 4.7 and gcc 4.8.1.-- Best regards, Dmitry Voronin#include stdio.h
#define ereport_domain(elevel, domain, rest) \
do { \
const int elevel_ = (elevel); \
if (errstart(elevel_, __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain)) \
errfinish rest; \
if (elevel_ = 2) \
pg_unreachable(); \
} while(0)
#define ereport(elevel, rest) \
ereport_domain(elevel, TEXTDOMAIN, rest)
#define TEXTDOMAIN NULL
#define PG_FUNCNAME_MACRO NULL
#define ERROR 20
#define errcontext set_errcontext_domain(TEXTDOMAIN), errcontext_msg
int set_errcontext_domain(const char *domain)
{
printf(set_errcontext_domain\n);
}
int
errstart(int elevel, const char *filename, int lineno,
const char *funcname, const char *domain)
{
printf(errstart\n);
return 1;
}
void pg_unreachable()
{
printf(pg_unreachable\n);
}
void errfinish(int dummy, ...)
{
printf(errfinish\n);
}
int
errcontext_msg(const char *fmt,...)
{
printf(errcontext_msg\n);
return 0;
}
int errmsg(const char *fmt, ...)
{
printf(errmsg\n);
return 0;
}
int main()
{
/* this is incorrect */
ereport(ERROR,
(errmsg(%s, error message),
errcontext(%s\nin PL/Tcl function \%s\,
test1, test2)));
printf(\n);
/* this is correct */
ereport(ERROR,
(errmsg(%s, error message),
(errcontext(%s\nin PL/Tcl function \%s\,
test1, test2;
return 0;
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Redesigning checkpoint_segments
On Fri, Jan 2, 2015 at 3:27 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: On 01/01/2015 03:24 AM, Josh Berkus wrote: Please remind me because I'm having trouble finding this in the archives: how does wal_keep_segments interact with the new settings? It's not very straightforward. First of all, min_recycle_wal_size has a different effect than wal_keep_segments. Raising min_recycle_wal_size causes more segments to be recycled rather than deleted, while wal_keep_segments causes old segments to be retained as old segments, so that they can be used for streaming replication. If you raise min_recycle_wal_size, it will not do any good for streaming replication. wal_keep_segments does not affect the calculation of CheckPointSegments. If you set wal_keep_segments high enough, checkpoint_wal_size will be exceeded. The other alternative would be to force a checkpoint earlier, i.e. lower CheckPointSegments, so that checkpoint_wal_size would be honored. However, if you set wal_keep_segments high enough, higher than checkpoint_wal_size, it's impossible to honor checkpoint_wal_size no matter how frequently you checkpoint. Doesn't this indicate that we should have some co-relation between checkpoint_wal_size and wal_keep_segments? It's not totally straightforward to calculate what maximum size of WAL a given wal_keep_segments settings will force. wal_keep_segments is taken into account at a checkpoint, when we recycle old WAL segments. For example, imagine that prior checkpoint started at segment 95, a new checkpoint finishes at segment 100, and wal_keep_segments=10. Because of wal_keep_segments, we have to retain segments 90-95, which could otherwise be recycled. So that forces a WAL size of 10 segments, while otherwise 5 would be enough. However, before we reach the next checkpoint, let's assume it will complete at segment 105, we will consume five more segments, so the actual max WAL size is 15 segments. However, we could start recycling the segments 90-95 before we reach the next checkpoint, because wal_keep_segments stipulates how many segments from the current *insert* location needs to be retained, with not regard to checkpoints. But we only attempt to recycle segments at checkpoints. I am thinking that it might make sense to have checkpoint_wal_size equal to size of wal_keep_segments incase wal_keep_segments is greater than checkpoint_wal_size size. It will not make any difference in retaining wal segments, but I think it can make checkpoint trigger at more appropriate intervals. Won't this help in addressing the above situation explained by you to an extent as it will make a new checkpoint to start little later such that it will help in removing segments between 90-95 one cycle earlier. So that could be made more straightforward if we recycled old segments in the background, between checkpoints. That might allow merging wal_keep_segments and min_recycle_wal_size settings, too: instead of renaming all old recycleable segments at a checkpoint, you could keep them around as old segments until they're actually needed for reuse, so they could be used for streaming replication up to that point. Are you imagining some other background process to do this activity? Does it make sense if we try to do the same in foreground (I understand that it can impact performance of that session, but such a thing can maintain the WAL size better)? With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] INSERT ... ON CONFLICT UPDATE and RLS
Dean, * Dean Rasheed (dean.a.rash...@gmail.com) wrote: On 10 January 2015 at 21:38, Dean Rasheed dean.a.rash...@gmail.com wrote: OK, I'll take a look at it. I started reading the existing code that expands RLS policies and spotted a couple of bugs that should be fixed first:- 1). In prepend_row_security_policies(), defaultDeny should be initialised to false at the top. 2). In fireRIRrules(), activeRIRs isn't being reset properly after recursing, which will cause a self-join to fail. So as I starting looking into these bugs, which looked fairly trivial, the test case I came up with revealed another more subtle bug with RLS, using a more obscure query -- given an update of the form UPDATE t1 ... FROM t2 ..., if t1 is part of an inheritance hierarchy and both t1 and t2 have RLS enabled, the inheritance planner was doing the wrong thing. There is code in there to copy subquery RTEs into each child plan, which needed to do the same for non-target RTEs with security barrier quals, which haven't yet been turned into subqueries. Similarly the rowmark preprocessing code needed to be taught about (non-target) RTEs with security barrier quals to handle this kind of UPDATE with a FROM clause. Interesting, thanks for the work! I had been suspicious that there was an issue with the recursion handling. The attached patch fixes those issues. Excellent. Will take a look. This bug can't happen with updatable security barrier views, since non-target security barrier views are expanded in the rewriter, so technically this doesn't need bach-patching to 9.4. However, I think the planner changes should probably be back-patched anyway, to keep the code in the 2 branches in sync, and more maintainable. Also it feels like the planner ought to be able to handle any legal query thrown at it, even if it looks like the 9.4 rewriter couldn't generate such a query. I'm not anxious to back-patch if there's no issue in existing branches, but I understand your point about making sure it can handle legal queries even if we don't believe current 9.4 can't generate them. We don't tend to back-patch just to keep things in sync as that could possibly have unintended side-effects. Looking at the regression tests a bit though, I notice that this removes the lower-level LockRows.. There had been much discussion about that last spring which I believe you were a part of..? I specifically recall discussing it with Craig, at least. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] ereport bug
Dmitry Voronin carriingfat...@yandex.ru writes: divdivdivdiv data-lang=2divHello, postgresmen!/divdiv/divdivI found incorrect execution of ereport() macro. br /If we pass into ereport() function 2 or more arguments, the macro errcontext does not correct execute. So, ereport() call stack is:/divdiv/divdiverrstartbr /errcontext_msgbr /set_errcontext_domainbr /errmsgbr /errfinishbr /pg_unreachable/divdiv/divdivspan lang=enspanThis bug/span spancauses that error messages (for example, in PL/TCL) are /span/spanspan lang=enspannot localized.br /br /Solutions:br /- Wrap all errcontext() macro in /span/spanspan lang=enspanspan lang=enspanbrackets, that is errcontext(error message %s, end message) -gt; (/span/span/span/spanspan lang=enspanspan lang=enspanerrcontext(error message %s, end message))/span/span/span/span/divdivspan lang=enspanspan lang=enspan- Rewrite this macro/span/spa! n/span/span/divdivspan lang=enspanspan lang=enspan- ???/span/span/span/span/divdiv/divdivspan lang=enspanI am attaching/span spanto this letter/span spana test case/span spanthat shows/span spanthe behavior errcontext() macro and the way to fix it.br //span/span/divdivbr /I am using postgresql 9.4 and test it on gcc 4.7 and gcc 4.8.1.br /br //divdiv-- Best regards, Dmitry Voronin/div/div/div/div/div (Please don't post HTML-only mail to the PG mailing lists ...) Hm ... the initial thought was that errcontext would never be used directly in an ereport() macro, but you're right that we now have some places that violate that rule. So the comma expression turns into a couple of arguments to errfinish, meaning the order of evaluation becomes compiler-dependent which is bad. I think the easiest fix is to have errstart initialize context_domain to the same value as domain. The order of evaluation is still compiler-dependent, but it no longer matters because any errcontext calls occurring textually within an ereport should be trying to select the same domain as the ereport 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] INSERT ... ON CONFLICT UPDATE and RLS
On 10 January 2015 at 21:38, Dean Rasheed dean.a.rash...@gmail.com wrote:
OK, I'll take a look at it. I started reading the existing code that
expands RLS policies and spotted a couple of bugs that should be fixed
first:-
1). In prepend_row_security_policies(), defaultDeny should be
initialised to false at the top.
2). In fireRIRrules(), activeRIRs isn't being reset properly after
recursing, which will cause a self-join to fail.
So as I starting looking into these bugs, which looked fairly trivial,
the test case I came up with revealed another more subtle bug with
RLS, using a more obscure query -- given an update of the form UPDATE
t1 ... FROM t2 ..., if t1 is part of an inheritance hierarchy and both
t1 and t2 have RLS enabled, the inheritance planner was doing the
wrong thing. There is code in there to copy subquery RTEs into each
child plan, which needed to do the same for non-target RTEs with
security barrier quals, which haven't yet been turned into subqueries.
Similarly the rowmark preprocessing code needed to be taught about
(non-target) RTEs with security barrier quals to handle this kind of
UPDATE with a FROM clause.
The attached patch fixes those issues.
This bug can't happen with updatable security barrier views, since
non-target security barrier views are expanded in the rewriter, so
technically this doesn't need bach-patching to 9.4. However, I think
the planner changes should probably be back-patched anyway, to keep
the code in the 2 branches in sync, and more maintainable. Also it
feels like the planner ought to be able to handle any legal query
thrown at it, even if it looks like the 9.4 rewriter couldn't generate
such a query.
Regards,
Dean
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
new file mode 100644
index 9cbbcfb..119a016
*** a/src/backend/optimizer/plan/planner.c
--- b/src/backend/optimizer/plan/planner.c
*** inheritance_planner(PlannerInfo *root)
*** 790,795
--- 790,796
{
Query *parse = root-parse;
int parentRTindex = parse-resultRelation;
+ Bitmapset *resultRTindexes = NULL;
List *final_rtable = NIL;
int save_rel_array_size = 0;
RelOptInfo **save_rel_array = NULL;
*** inheritance_planner(PlannerInfo *root)
*** 814,820
--- 815,835
* (1) would result in a rangetable of length O(N^2) for N targets, with
* at least O(N^3) work expended here; and (2) would greatly complicate
* management of the rowMarks list.
+ *
+ * Note that any RTEs with security barrier quals will be turned into
+ * subqueries during planning, and so we must create copies of them too,
+ * except where they are target relations, which will each only be used
+ * in a single plan.
*/
+ resultRTindexes = bms_add_member(resultRTindexes, parentRTindex);
+ foreach(lc, root-append_rel_list)
+ {
+ AppendRelInfo *appinfo = (AppendRelInfo *) lfirst(lc);
+ if (appinfo-parent_relid == parentRTindex)
+ resultRTindexes = bms_add_member(resultRTindexes,
+ appinfo-child_relid);
+ }
+
foreach(lc, root-append_rel_list)
{
AppendRelInfo *appinfo = (AppendRelInfo *) lfirst(lc);
*** inheritance_planner(PlannerInfo *root)
*** 885,905
{
RangeTblEntry *rte = (RangeTblEntry *) lfirst(lr);
! if (rte-rtekind == RTE_SUBQUERY)
{
Index newrti;
/*
* The RTE can't contain any references to its own RT
! * index, so we can save a few cycles by applying
! * ChangeVarNodes before we append the RTE to the
! * rangetable.
*/
newrti = list_length(subroot.parse-rtable) + 1;
ChangeVarNodes((Node *) subroot.parse, rti, newrti, 0);
ChangeVarNodes((Node *) subroot.rowMarks, rti, newrti, 0);
ChangeVarNodes((Node *) subroot.append_rel_list, rti, newrti, 0);
rte = copyObject(rte);
subroot.parse-rtable = lappend(subroot.parse-rtable,
rte);
}
--- 900,928
{
RangeTblEntry *rte = (RangeTblEntry *) lfirst(lr);
! /*
! * Copy subquery RTEs and RTEs with security barrier quals
! * that will be turned into subqueries, except those that are
! * target relations.
! */
! if (rte-rtekind == RTE_SUBQUERY ||
! (rte-securityQuals != NIL
! !bms_is_member(rti, resultRTindexes)))
{
Index newrti;
/*
* The RTE can't contain any references to its own RT
! * index, except in the security barrier quals, so we can
! * save a few cycles by applying ChangeVarNodes before we
! * append the RTE to the rangetable.
*/
newrti = list_length(subroot.parse-rtable) + 1;
ChangeVarNodes((Node *) subroot.parse, rti, newrti, 0);
ChangeVarNodes((Node *) subroot.rowMarks, rti, newrti, 0);
ChangeVarNodes((Node *) subroot.append_rel_list, rti, newrti, 0);
rte = copyObject(rte);
+ ChangeVarNodes((Node *)
Re: [HACKERS] Latches and barriers
On 2015-01-12 11:03:42 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: While it might not be required for existing latch uses (I'm *not* sure that's true) I think at least syncrep.c might not be correct. In SyncRepWakeQueue() it sets PGPROC-syncRepState without the necessary barriers (via locks), although it does use them in SyncRepWaitForLSN(). It is, perhaps surprisingly to many, not sufficient to take a spinlock, change the flag, release it and then set the latch - the release alone doesn't guarantee a sufficient barrier unless looking at the flag is also protected by the spinlock. I still think that we should fix those XXX by actually using barriers now that we have them. I don't think we want every callsite worry about using barriers. Agreed? Yeah, now that we have barrier code we think works, we should definitely put some in there. The only reason it's like that is we didn't have any real barrier support at the time. Master only though? If we decide we need it earlier, we can backport that commit lateron... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Latches and barriers
Hi,
latch.h has the following comment:
* Presently, when using a shared latch for interprocess signalling, the
* flag variable(s) set by senders and inspected by the wait loop must
* be protected by spinlocks or LWLocks, else it is possible to miss events
* on machines with weak memory ordering (such as PPC). This restriction
* will be lifted in future by inserting suitable memory barriers into
* SetLatch and ResetLatch.
and unix_latch.c has:
SetLatch(volatile Latch *latch)
{
pid_t owner_pid;
/*
* XXX there really ought to be a memory barrier operation right here,
to
* ensure that any flag variables we might have changed get flushed to
* main memory before we check/set is_set. Without that, we have to
* require that callers provide their own synchronization for machines
* with weak memory ordering (see latch.h).
*/
/* Quick exit if already set */
if (latch-is_set)
return;
...
void
ResetLatch(volatile Latch *latch)
{
/* Only the owner should reset the latch */
Assert(latch-owner_pid == MyProcPid);
latch-is_set = false;
/*
* XXX there really ought to be a memory barrier operation right here,
to
* ensure that the write to is_set gets flushed to main memory before we
* examine any flag variables. Otherwise a concurrent SetLatch might
* falsely conclude that it needn't signal us, even though we have
missed
* seeing some flag updates that SetLatch was supposed to inform us of.
* For the moment, callers must supply their own synchronization of flag
* variables (see latch.h).
*/
}
Triggered by another thread I converted proc.c and lwlock.c to use
latches for blocking. Which worked fine on my laptop, but failed
miserably, often within less than a second, on my 2 socket x86
workstation. After a fair amount of headscratching I figured out that
it's indeed those missing barriers. Adding them made it work.
Thinking about it, it's not too surprising. PGPROC's lwWaiting and
procLatch aren't at the same address (more specifically on a different
cacheline). X86 allows reordering of loads with stores to different
addresses. That's what happening here.
While it might not be required for existing latch uses (I'm *not* sure
that's true), I still think that we should fix those XXX by actually
using barriers now that we have them. I don't think we want every
callsite worry about using barriers.
Agreed?
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] parallel mode and parallel contexts
On Wed, Jan 7, 2015 at 3:54 PM, Jim Nasby jim.na...@bluetreble.com wrote: Agreed, I was more concerned with calls to nextval(), which don't seem to be prevented in parallel mode? It looks prevented: /* * Forbid this during parallel operation because, to make it work, * the cooperating backends would need to share the backend-local cached * sequence information. Currently, we don't support that. */ PreventCommandIfParallelMode(nextval()); I was more thinking about all the add-on pl's like pl/ruby. But yeah, I don't see that there's much we can do if they're not using SPI. Actually, there is: it's forbidden at the layer of heap_insert(), heap_update(), heap_delete(). It's hard to imagine anyone trying to modify the database as a lower level than that. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] parallel mode and parallel contexts
On Thu, Jan 8, 2015 at 6:52 AM, Amit Kapila amit.kapil...@gmail.com wrote: + seg = dsm_attach(DatumGetInt32(main_arg)); Here, I think DatumGetUInt32() needs to be used instead of DatumGetInt32() as the segment handle is uint32. OK, I'll change that in the next version. -- 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] INSERT ... ON CONFLICT UPDATE and RLS
On 12 January 2015 at 14:24, Stephen Frost sfr...@snowman.net wrote:
Looking at the regression tests a bit though, I notice that this removes
the lower-level LockRows.. There had been much discussion about that
last spring which I believe you were a part of..? I specifically recall
discussing it with Craig, at least.
Ah, yes you're right. Looking back over that discussion it shouldn't
be removing those lower-level LockRows. I was a bit aggressive with my
change to the rowmark preprocessing -- the first loop applies to
requested locks, like SELECT .. FOR UPDATE, so it shouldn't be messing
with that, presecurity.c handles that fine. It's only the second loop
that needs to be taught about RTEs with security quals that will
become subqueries.
Here's an updated patch, that passes with the original regression test results.
Regards,
Dean
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
new file mode 100644
index 9cbbcfb..34ddf41
*** a/src/backend/optimizer/plan/planner.c
--- b/src/backend/optimizer/plan/planner.c
*** inheritance_planner(PlannerInfo *root)
*** 790,795
--- 790,796
{
Query *parse = root-parse;
int parentRTindex = parse-resultRelation;
+ Bitmapset *resultRTindexes = NULL;
List *final_rtable = NIL;
int save_rel_array_size = 0;
RelOptInfo **save_rel_array = NULL;
*** inheritance_planner(PlannerInfo *root)
*** 814,820
--- 815,835
* (1) would result in a rangetable of length O(N^2) for N targets, with
* at least O(N^3) work expended here; and (2) would greatly complicate
* management of the rowMarks list.
+ *
+ * Note that any RTEs with security barrier quals will be turned into
+ * subqueries during planning, and so we must create copies of them too,
+ * except where they are target relations, which will each only be used
+ * in a single plan.
*/
+ resultRTindexes = bms_add_member(resultRTindexes, parentRTindex);
+ foreach(lc, root-append_rel_list)
+ {
+ AppendRelInfo *appinfo = (AppendRelInfo *) lfirst(lc);
+ if (appinfo-parent_relid == parentRTindex)
+ resultRTindexes = bms_add_member(resultRTindexes,
+ appinfo-child_relid);
+ }
+
foreach(lc, root-append_rel_list)
{
AppendRelInfo *appinfo = (AppendRelInfo *) lfirst(lc);
*** inheritance_planner(PlannerInfo *root)
*** 885,905
{
RangeTblEntry *rte = (RangeTblEntry *) lfirst(lr);
! if (rte-rtekind == RTE_SUBQUERY)
{
Index newrti;
/*
* The RTE can't contain any references to its own RT
! * index, so we can save a few cycles by applying
! * ChangeVarNodes before we append the RTE to the
! * rangetable.
*/
newrti = list_length(subroot.parse-rtable) + 1;
ChangeVarNodes((Node *) subroot.parse, rti, newrti, 0);
ChangeVarNodes((Node *) subroot.rowMarks, rti, newrti, 0);
ChangeVarNodes((Node *) subroot.append_rel_list, rti, newrti, 0);
rte = copyObject(rte);
subroot.parse-rtable = lappend(subroot.parse-rtable,
rte);
}
--- 900,928
{
RangeTblEntry *rte = (RangeTblEntry *) lfirst(lr);
! /*
! * Copy subquery RTEs and RTEs with security barrier quals
! * that will be turned into subqueries, except those that are
! * target relations.
! */
! if (rte-rtekind == RTE_SUBQUERY ||
! (rte-securityQuals != NIL
! !bms_is_member(rti, resultRTindexes)))
{
Index newrti;
/*
* The RTE can't contain any references to its own RT
! * index, except in the security barrier quals, so we can
! * save a few cycles by applying ChangeVarNodes before we
! * append the RTE to the rangetable.
*/
newrti = list_length(subroot.parse-rtable) + 1;
ChangeVarNodes((Node *) subroot.parse, rti, newrti, 0);
ChangeVarNodes((Node *) subroot.rowMarks, rti, newrti, 0);
ChangeVarNodes((Node *) subroot.append_rel_list, rti, newrti, 0);
rte = copyObject(rte);
+ ChangeVarNodes((Node *) rte-securityQuals, rti, newrti, 0);
subroot.parse-rtable = lappend(subroot.parse-rtable,
rte);
}
*** preprocess_rowmarks(PlannerInfo *root)
*** 2254,2261
newrc = makeNode(PlanRowMark);
newrc-rti = newrc-prti = i;
newrc-rowmarkId = ++(root-glob-lastRowMarkId);
! /* real tables support REFERENCE, anything else needs COPY */
if (rte-rtekind == RTE_RELATION
rte-relkind != RELKIND_FOREIGN_TABLE)
newrc-markType = ROW_MARK_REFERENCE;
else
--- 2277,2289
newrc = makeNode(PlanRowMark);
newrc-rti = newrc-prti = i;
newrc-rowmarkId = ++(root-glob-lastRowMarkId);
! /*
! * Real tables support REFERENCE, anything else needs COPY. Since
! * RTEs with security barrier quals will become subqueries, they also
! * need COPY.
! */
if (rte-rtekind ==
Re: [HACKERS] To do for psql to show installable extensions
Jeff Janes wrote: I'd like to propose a wiki to-do item for a backslash command in psql which would show all installable extensions, basically just a wrapper around 'select * from pg_available_extensions'. I've wanted it a few times recently, mostly in testing. +1. Any reason this wouldn't be desirable? No idea. I guess if pg_available_extensions is acceptable, a \-command should be acceptable as well. But you might as well look up the old discussions that led to the current situation where we have an SRF and not a \-command. What should it be called? \dxx / \dxi ? As long as it shows in \dxtab I am fine with almost anything sensible, really. I thought of \dx+, but the + is already used to show the objects associated with the extensions. (Althought it seems like it would more in keeping with other usage if \dx+ only listed the objects if it was given a pattern, and did what I propose if given no pattern) I hate the pattern/no pattern discrepancy -- I vote not to propagate it any further. -- Á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
[HACKERS] PGCon 2015 call for papers - reminder
A reminder, only about a week to submit your proposal: http://www.pgcon.org/2015/papers.php PGCon 2015 will be on 18-19 June 2015 at University of Ottawa. * 16-17 (Tue-Wed) tutorials * 18-19 (Thu-Fri) talks - the main part of the conference * 20 (Sat) The Unconference (very popular) PLEASE NOTE: PGCon 2015 is in June. See http://www.pgcon.org/2015/ We are now accepting proposals for the main part of the conference (18-19 June). Proposals can be quite simple. We do not require academic-style papers. You have about two weeks left before submissions close. If you are doing something interesting with PostgreSQL, please submit a proposal. You might be one of the backend hackers or work on a PostgreSQL related project and want to share your know-how with others. You might be developing an interesting system using PostgreSQL as the foundation. Perhaps you migrated from another database to PostgreSQL and would like to share details. These, and other stories are welcome. Both users and developers are encouraged to share their experiences. Here are a some ideas to jump start your proposal process: - novel ways in which PostgreSQL is used - migration of production systems from another database - data warehousing - tuning PostgreSQL for different work loads - replication and clustering - hacking the PostgreSQL code - PostgreSQL derivatives and forks - applications built around PostgreSQL - benchmarking and performance engineering - case studies - location-aware and mapping software with PostGIS - The latest PostgreSQL features and features in development - research and teaching with PostgreSQL - things the PostgreSQL project could do better - integrating PostgreSQL with 3rd-party software Both users and developers are encouraged to share their experiences. The schedule is: 1 Dec 2014 Proposal acceptance begins 19 Jan 2015 Proposal acceptance ends 19 Feb 2015 Confirmation of accepted proposals NOTE: the call for lightning talks will go out very close to the conference. Do not submit lightning talks proposals until then. See also http://www.pgcon.org/2015/papers.php Instructions for submitting a proposal to PGCon 2015 are available from: http://www.pgcon.org/2015/submissions.php — Dan Langille http://langille.org/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] To do for psql to show installable extensions
I'd like to propose a wiki to-do item for a backslash command in psql which would show all installable extensions, basically just a wrapper around 'select * from pg_available_extensions'. I've wanted it a few times recently, mostly in testing. Any reason this wouldn't be desirable? What should it be called? I thought of \dx+, but the + is already used to show the objects associated with the extensions. (Althought it seems like it would more in keeping with other usage if \dx+ only listed the objects if it was given a pattern, and did what I propose if given no pattern) Cheers, Jeff
Re: [HACKERS] To do for psql to show installable extensions
* Jeff Janes (jeff.ja...@gmail.com) wrote: I'd like to propose a wiki to-do item for a backslash command in psql which would show all installable extensions, basically just a wrapper around 'select * from pg_available_extensions'. I guess I don't feel very strongly for or against adding a backslash command for this, but just wanted to mention that you can use table, as in: table pg_available_extensions; Slightly shorter. :) Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] To do for psql to show installable extensions
Alvaro Herrera alvhe...@2ndquadrant.com writes: Jeff Janes wrote: I thought of \dx+, but the + is already used to show the objects associated with the extensions. (Althought it seems like it would more in keeping with other usage if \dx+ only listed the objects if it was given a pattern, and did what I propose if given no pattern) I hate the pattern/no pattern discrepancy -- I vote not to propagate it any further. The set of things that is known about an installed extension is quite a bit different from what is known about an uninstalled-but-available one. To make \dx print both categories would require dumbing it down to print only the intersection of those things, or else some fancy footwork and a lot of NULL column values. -1 for that. (This is exactly why pg_available_extensions is separate from pg_extension in the first place.) I'm okay with inventing some new command like \dxu or \dxa (mnemonic uninstalled or available respectively). 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] To do for psql to show installable extensions
Dne 12.1.2015 22:26 Tom Lane t...@sss.pgh.pa.us napsal(a): Alvaro Herrera alvhe...@2ndquadrant.com writes: Jeff Janes wrote: I thought of \dx+, but the + is already used to show the objects associated with the extensions. (Althought it seems like it would more in keeping with other usage if \dx+ only listed the objects if it was given a pattern, and did what I propose if given no pattern) I hate the pattern/no pattern discrepancy -- I vote not to propagate it any further. The set of things that is known about an installed extension is quite a bit different from what is known about an uninstalled-but-available one. To make \dx print both categories would require dumbing it down to print only the intersection of those things, or else some fancy footwork and a lot of NULL column values. -1 for that. (This is exactly why pg_available_extensions is separate from pg_extension in the first place.) I'm okay with inventing some new command like \dxu or \dxa (mnemonic uninstalled or available respectively). I like \dxa Regards Pavel 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] max_connections documentation
On 1/10/15 12:06 AM, Amit Kapila wrote: On Sat, Jan 10, 2015 at 6:20 AM, Jim Nasby jim.na...@bluetreble.com mailto:jim.na...@bluetreble.com wrote: I'm surprised to see that the docs make no mention of how max_connections, max_worker_processes and autovacuum_max_workers (don't) relate. I couldn't remember and had to actually look at the code. I'd like to clarify this in the max_connecitons section of the documents by doing s/connections/user connections/ and including the formula for MaxBackends (MaxBackends = MaxConnections + autovacuum_max_workers + 1 + max_worker_processes). I'll also mention that any postgres_fdw connections are considered user connections. I think it makes sense to add such a clarification in docs, however not sure if specifying along with max_connections parameter is good idea as the formula is somewhat internal and is not directly related to max_connections. It's not all that internal; it directly controls how many entries you could end up with in pg_stat_activity. Certainly monitoring software needs to be aware of how this stuff works, and I think many DBAs would want to know as well. Maybe we don't need the formula itself, but we need to explain what backends do not count towards max_connections (and ideally how to identify them). Maybe the best thing would be to add an internal field to pg_stat_activity to indicate what backends were internal and not user-related... How about specifying in below page: http://www.postgresql.org/docs/devel/static/connect-estab.html I think someone that's concerned about connection limits is much more likely to look at the config section of the docs than that section, so we'd need to cross-reference them. Which would probably be good to do anyway... -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] To do for psql to show installable extensions
On Mon, Jan 12, 2015 at 01:05:16PM -0800, Jeff Janes wrote: I'd like to propose a wiki to-do item for a backslash command in psql which would show all installable extensions, basically just a wrapper around 'select * from pg_available_extensions'. I've wanted it a few times recently, mostly in testing. If your psql has libreadline, you can CREATE EXTENSION tabtab and get a list. It doesn't distinguish between installed ones and available, though. Any reason this wouldn't be desirable? What should it be called? I thought of \dx+, but the + is already used to show the objects associated with the extensions. (Althought it seems like it would more in keeping with other usage if \dx+ only listed the objects if it was given a pattern, and did what I propose if given no pattern) For what it's worth, of the proposals so far, I like \dxa most. Cheers, David. -- David Fetter da...@fetter.org http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Hash Function
On 1/11/15 8:52 AM, Ravi Kiran wrote: Hi, I want to know what kind of hash function postgres is using currently, can someone please explain the algorithm postgres is using for the hash function in the hash join algorithm. That's ultimately going to be determined by the operator family of the data types involved in the join, but generally stuff uses the internal hash function which is a modified Jenkins hash. BTW, I just investigated switching the algorithm we use for buffer hashing [1]. Turned out to be a dead end, but it's very possible that a newer hash than Jenkins (ie: CityHash or FarmHash) would be a win for larger data. [1] http://www.postgresql.org/message-id/24160.1419460...@sss.pgh.pa.us -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WITH CHECK and Column-Level Privileges
Dean, Robert,
* Dean Rasheed (dean.a.rash...@gmail.com) wrote:
On 29 October 2014 13:04, Stephen Frost sfr...@snowman.net wrote:
* Robert Haas (robertmh...@gmail.com) wrote:
On Wed, Oct 29, 2014 at 8:16 AM, Stephen Frost sfr...@snowman.net wrote:
suggestions. If the user does not have table-level SELECT rights,
they'll see for the Failing row contains case, they'll get:
Failing row contains (col1, col2, col3) = (1, 2, 3).
Or, if they have no access to any columns:
Failing row contains () = ()
I haven't looked at the code, but that sounds nice, except that if
they have no access to any columns, I'd leave the message out
altogether instead of emitting it with no useful content.
Alright, I can change things around to make that happen without too much
trouble.
Yes, skim-reading the patch, it looks like a good approach to me, and
also +1 for not emitting anything if no values are visible.
Alright, here's an updated patch which doesn't return any detail if no
values are visible or if only a partial key is visible.
Please take a look. I'm not thrilled with simply returning an empty
string and then checking that for BuildIndexValueDescription and
ExecBuildSlotValueDescription, but I figured we might have other users
of BuildIndexValueDescription and I wasn't seeing a particularly better
solution. Suggestions welcome, of course.
Thanks!
Stephen
From e00cc2f5be4524989e8d670296f82bb99f3774a1 Mon Sep 17 00:00:00 2001
From: Stephen Frost sfr...@snowman.net
Date: Mon, 12 Jan 2015 17:04:11 -0500
Subject: [PATCH] Fix column-privilege leak in error-message paths
While building error messages to return to the user,
BuildIndexValueDescription and ExecBuildSlotValueDescription would
happily include the entire key or entire row in the result returned to
the user, even if the user didn't have access to view all of the columns
being included.
Instead, include only those columns which the user is providing or which
the user has select rights on. If the user does not have any rights
to view the table or any of the columns involved then no detail is
provided. Note that, for duplicate key cases, the user must have access
to all of the columns for the key to be shown; a partial key will not be
returned.
Back-patch all the way, as column-level privileges are now in all
supported versions.
---
src/backend/access/index/genam.c | 59 -
src/backend/access/nbtree/nbtinsert.c| 30 +++--
src/backend/commands/copy.c | 6 +-
src/backend/executor/execMain.c | 215 +++
src/backend/executor/execUtils.c | 12 +-
src/backend/utils/sort/tuplesort.c | 28 ++--
src/test/regress/expected/privileges.out | 31 +
src/test/regress/sql/privileges.sql | 24
8 files changed, 328 insertions(+), 77 deletions(-)
diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c
index 850008b..1090568 100644
--- a/src/backend/access/index/genam.c
+++ b/src/backend/access/index/genam.c
@@ -25,10 +25,12 @@
#include lib/stringinfo.h
#include miscadmin.h
#include storage/bufmgr.h
+#include utils/acl.h
#include utils/builtins.h
#include utils/lsyscache.h
#include utils/rel.h
#include utils/snapmgr.h
+#include utils/syscache.h
#include utils/tqual.h
@@ -154,6 +156,9 @@ IndexScanEnd(IndexScanDesc scan)
* form (key_name, ...)=(key_value, ...). This is currently used
* for building unique-constraint and exclusion-constraint error messages.
*
+ * Note that if the user does not have permissions to view the columns
+ * involved, an empty string will be returned instead.
+ *
* The passed-in values/nulls arrays are the raw input to the index AM,
* e.g. results of FormIndexDatum --- this is not necessarily what is stored
* in the index, but it's what the user perceives to be stored.
@@ -163,13 +168,63 @@ BuildIndexValueDescription(Relation indexRelation,
Datum *values, bool *isnull)
{
StringInfoData buf;
+ Form_pg_index idxrec;
+ HeapTuple ht_idx;
int natts = indexRelation-rd_rel-relnatts;
int i;
+ int keyno;
+ Oid indexrelid = RelationGetRelid(indexRelation);
+ Oid indrelid;
+ AclResult aclresult;
+
+ /*
+ * Check permissions- if the users does not have access to view the
+ * data in the key columns, we return instead, which callers can
+ * test for and use or not accordingly.
+ *
+ * First we need to check table-level SELECT access and then, if
+ * there is no access there, check column-level permissions.
+ */
+
+ /*
+ * Fetch the pg_index tuple by the Oid of the index
+ */
+ ht_idx = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexrelid));
+ if (!HeapTupleIsValid(ht_idx))
+ elog(ERROR, cache lookup failed for index %u, indexrelid);
+ idxrec = (Form_pg_index) GETSTRUCT(ht_idx);
+
+ indrelid = idxrec-indrelid;
+ Assert(indexrelid == idxrec-indexrelid);
+
+ /* Table-level SELECT is enough, if the user has it
Re: [HACKERS] Removing INNER JOINs
On 12/3/14 1:08 PM, Tom Lane wrote: Heikki Linnakangashlinnakan...@vmware.com writes: Do you need to plan for every combination, where some joins are removed and some are not? I would vote for just having two plans and one switch node. To exploit any finer grain, we'd have to have infrastructure that would let us figure out*which* constraints pending triggers might indicate transient invalidity of, and that doesn't seem likely to be worth the trouble. In the interest of keeping the first pass simple... what if there was simply a switch node in front of every join that could be removable? That means you'd still be stuck with the overall plan you got from not removing anything, but simply eliminating the access to the relation(s) might be a big win in many cases, and presumably this would be a lot easier to code. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WITH CHECK and Column-Level Privileges
All,
Apologies, forgot to mention- this is again 9.4.
Thanks,
Stephen
* Stephen Frost (sfr...@snowman.net) wrote:
Dean, Robert,
* Dean Rasheed (dean.a.rash...@gmail.com) wrote:
On 29 October 2014 13:04, Stephen Frost sfr...@snowman.net wrote:
* Robert Haas (robertmh...@gmail.com) wrote:
On Wed, Oct 29, 2014 at 8:16 AM, Stephen Frost sfr...@snowman.net
wrote:
suggestions. If the user does not have table-level SELECT rights,
they'll see for the Failing row contains case, they'll get:
Failing row contains (col1, col2, col3) = (1, 2, 3).
Or, if they have no access to any columns:
Failing row contains () = ()
I haven't looked at the code, but that sounds nice, except that if
they have no access to any columns, I'd leave the message out
altogether instead of emitting it with no useful content.
Alright, I can change things around to make that happen without too much
trouble.
Yes, skim-reading the patch, it looks like a good approach to me, and
also +1 for not emitting anything if no values are visible.
Alright, here's an updated patch which doesn't return any detail if no
values are visible or if only a partial key is visible.
Please take a look. I'm not thrilled with simply returning an empty
string and then checking that for BuildIndexValueDescription and
ExecBuildSlotValueDescription, but I figured we might have other users
of BuildIndexValueDescription and I wasn't seeing a particularly better
solution. Suggestions welcome, of course.
Thanks!
Stephen
From e00cc2f5be4524989e8d670296f82bb99f3774a1 Mon Sep 17 00:00:00 2001
From: Stephen Frost sfr...@snowman.net
Date: Mon, 12 Jan 2015 17:04:11 -0500
Subject: [PATCH] Fix column-privilege leak in error-message paths
While building error messages to return to the user,
BuildIndexValueDescription and ExecBuildSlotValueDescription would
happily include the entire key or entire row in the result returned to
the user, even if the user didn't have access to view all of the columns
being included.
Instead, include only those columns which the user is providing or which
the user has select rights on. If the user does not have any rights
to view the table or any of the columns involved then no detail is
provided. Note that, for duplicate key cases, the user must have access
to all of the columns for the key to be shown; a partial key will not be
returned.
Back-patch all the way, as column-level privileges are now in all
supported versions.
---
src/backend/access/index/genam.c | 59 -
src/backend/access/nbtree/nbtinsert.c| 30 +++--
src/backend/commands/copy.c | 6 +-
src/backend/executor/execMain.c | 215
+++
src/backend/executor/execUtils.c | 12 +-
src/backend/utils/sort/tuplesort.c | 28 ++--
src/test/regress/expected/privileges.out | 31 +
src/test/regress/sql/privileges.sql | 24
8 files changed, 328 insertions(+), 77 deletions(-)
diff --git a/src/backend/access/index/genam.c
b/src/backend/access/index/genam.c
index 850008b..1090568 100644
--- a/src/backend/access/index/genam.c
+++ b/src/backend/access/index/genam.c
@@ -25,10 +25,12 @@
#include lib/stringinfo.h
#include miscadmin.h
#include storage/bufmgr.h
+#include utils/acl.h
#include utils/builtins.h
#include utils/lsyscache.h
#include utils/rel.h
#include utils/snapmgr.h
+#include utils/syscache.h
#include utils/tqual.h
@@ -154,6 +156,9 @@ IndexScanEnd(IndexScanDesc scan)
* form (key_name, ...)=(key_value, ...). This is currently used
* for building unique-constraint and exclusion-constraint error messages.
*
+ * Note that if the user does not have permissions to view the columns
+ * involved, an empty string will be returned instead.
+ *
* The passed-in values/nulls arrays are the raw input to the index AM,
* e.g. results of FormIndexDatum --- this is not necessarily what is stored
* in the index, but it's what the user perceives to be stored.
@@ -163,13 +168,63 @@ BuildIndexValueDescription(Relation indexRelation,
Datum *values, bool *isnull)
{
StringInfoData buf;
+ Form_pg_index idxrec;
+ HeapTuple ht_idx;
int natts = indexRelation-rd_rel-relnatts;
int i;
+ int keyno;
+ Oid indexrelid = RelationGetRelid(indexRelation);
+ Oid indrelid;
+ AclResult aclresult;
+
+ /*
+ * Check permissions- if the users does not have access to view the
+ * data in the key columns, we return instead, which callers can
+ * test for and use or not accordingly.
+ *
+ * First we need to check table-level SELECT access and then, if
+ * there is no access
Re: [HACKERS] Safe memory allocation functions
Michael Paquier michael.paqu...@gmail.com writes: Attached is a patch adding the following set of functions for frontend and backends returning NULL instead of reporting ERROR when allocation fails: - palloc_safe - palloc0_safe - repalloc_safe Unimpressed with this naming convention. _unsafe would be nearer the mark ;-) 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] Comment typo in src/backend/executor/execMain.c
On 2015/01/10 1:08, Stephen Frost wrote: * Etsuro Fujita (fujita.ets...@lab.ntt.co.jp) wrote: I ran into a comment type. Please find attached a patch. Fix pushed. Thanks! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Safe memory allocation functions
Hi all,
For the last couple of weeks it has been mentioned a couple of times
that it would be useful to have a set of palloc APIs able to return
NULL on OOM to allow certain code paths to not ERROR and to take
another route when memory is under pressure. This has been for example
mentioned on the FPW compression thread or here:
http://www.postgresql.org/message-id/cab7npqrbewhsbj_tkaogtpcmrxyjsvkkb9p030d0tpijb4t...@mail.gmail.com
Attached is a patch adding the following set of functions for frontend
and backends returning NULL instead of reporting ERROR when allocation
fails:
- palloc_safe
- palloc0_safe
- repalloc_safe
This has simply needed some refactoring in aset.c to set up the new
functions by passing an additional control flag, and I didn't think
that adding a new safe version for AllocSetContextCreate was worth it.
Those APIs are not called anywhere yet, but I could for example write
a small extension for that that could be put in src/test/modules or
publish on github in my plugin repo. Also, I am not sure if this is
material for 9.5, even if the patch is not complicated, but let me
know if you are interested in it and I'll add it to the next CF.
Regards,
--
Michael
From 008f6bf5a1691fbdf59004157ed521d3b8c41eaf Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@otacoo.com
Date: Tue, 13 Jan 2015 15:40:38 +0900
Subject: [PATCH] Add safe memory allocation APIs able to return NULL on OOM
The following functions are added to the existing set for frontend and
backend:
- palloc_safe
- palloc0_safe
- repalloc_safe
---
src/backend/utils/mmgr/aset.c| 529 +++
src/backend/utils/mmgr/mcxt.c| 124 +
src/common/fe_memutils.c | 72 --
src/include/common/fe_memutils.h | 3 +
src/include/nodes/memnodes.h | 2 +
src/include/utils/palloc.h | 3 +
6 files changed, 451 insertions(+), 282 deletions(-)
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 85b3c9a..5911c53 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -243,11 +243,22 @@ typedef struct AllocChunkData
((AllocPointer)(((char *)(chk)) + ALLOC_CHUNKHDRSZ))
/*
+ * Wrappers for allocation functions.
+ */
+static void *set_alloc_internal(MemoryContext context,
+Size size, bool is_safe);
+static void *set_realloc_internal(MemoryContext context, void *pointer,
+ Size size, bool is_safe);
+
+/*
* These functions implement the MemoryContext API for AllocSet contexts.
*/
static void *AllocSetAlloc(MemoryContext context, Size size);
+static void *AllocSetAllocSafe(MemoryContext context, Size size);
static void AllocSetFree(MemoryContext context, void *pointer);
static void *AllocSetRealloc(MemoryContext context, void *pointer, Size size);
+static void *AllocSetReallocSafe(MemoryContext context,
+ void *pointer, Size size);
static void AllocSetInit(MemoryContext context);
static void AllocSetReset(MemoryContext context);
static void AllocSetDelete(MemoryContext context);
@@ -264,8 +275,10 @@ static void AllocSetCheck(MemoryContext context);
*/
static MemoryContextMethods AllocSetMethods = {
AllocSetAlloc,
+ AllocSetAllocSafe,
AllocSetFree,
AllocSetRealloc,
+ AllocSetReallocSafe,
AllocSetInit,
AllocSetReset,
AllocSetDelete,
@@ -517,140 +530,16 @@ AllocSetContextCreate(MemoryContext parent,
}
/*
- * AllocSetInit
- * Context-type-specific initialization routine.
- *
- * This is called by MemoryContextCreate() after setting up the
- * generic MemoryContext fields and before linking the new context
- * into the context tree. We must do whatever is needed to make the
- * new context minimally valid for deletion. We must *not* risk
- * failure --- thus, for example, allocating more memory is not cool.
- * (AllocSetContextCreate can allocate memory when it gets control
- * back, however.)
- */
-static void
-AllocSetInit(MemoryContext context)
-{
- /*
- * Since MemoryContextCreate already zeroed the context node, we don't
- * have to do anything here: it's already OK.
- */
-}
-
-/*
- * AllocSetReset
- * Frees all memory which is allocated in the given set.
- *
- * Actually, this routine has some discretion about what to do.
- * It should mark all allocated chunks freed, but it need not necessarily
- * give back all the resources the set owns. Our actual implementation is
- * that we hang onto any keeper block specified for the set. In this way,
- * we don't thrash malloc() when a context is repeatedly reset after small
- * allocations, which is typical behavior for per-tuple contexts.
- */
-static void
-AllocSetReset(MemoryContext context)
-{
- AllocSet set = (AllocSet) context;
- AllocBlock block;
-
- AssertArg(AllocSetIsValid(set));
-
-#ifdef MEMORY_CONTEXT_CHECKING
- /* Check for corruption and leaks before freeing */
- AllocSetCheck(context);
-#endif
-
- /* Clear chunk freelists */
- MemSetAligned(set-freelist, 0, sizeof(set-freelist));
-
- block =
[HACKERS] Unused variables in hstore_to_jsonb
Hi all,
Coverity pointed out that hstore_to_jsonb in hstore_io.c does not use
a couple of return values from pushJsonbValue.
Attached is a patch to fix that.
Regards,
--
Michael
*** a/contrib/hstore/hstore_io.c
--- b/contrib/hstore/hstore_io.c
***
*** 1338,1344 hstore_to_jsonb(PG_FUNCTION_ARGS)
JsonbParseState *state = NULL;
JsonbValue *res;
! res = pushJsonbValue(state, WJB_BEGIN_OBJECT, NULL);
for (i = 0; i count; i++)
{
--- 1338,1344
JsonbParseState *state = NULL;
JsonbValue *res;
! pushJsonbValue(state, WJB_BEGIN_OBJECT, NULL);
for (i = 0; i count; i++)
{
***
*** 1349,1355 hstore_to_jsonb(PG_FUNCTION_ARGS)
key.val.string.len = HS_KEYLEN(entries, i);
key.val.string.val = HS_KEY(entries, base, i);
! res = pushJsonbValue(state, WJB_KEY, key);
if (HS_VALISNULL(entries, i))
{
--- 1349,1355
key.val.string.len = HS_KEYLEN(entries, i);
key.val.string.val = HS_KEY(entries, base, i);
! pushJsonbValue(state, WJB_KEY, key);
if (HS_VALISNULL(entries, i))
{
***
*** 1361,1367 hstore_to_jsonb(PG_FUNCTION_ARGS)
val.val.string.len = HS_VALLEN(entries, i);
val.val.string.val = HS_VAL(entries, base, i);
}
! res = pushJsonbValue(state, WJB_VALUE, val);
}
res = pushJsonbValue(state, WJB_END_OBJECT, NULL);
--- 1361,1367
val.val.string.len = HS_VALLEN(entries, i);
val.val.string.val = HS_VAL(entries, base, i);
}
! pushJsonbValue(state, WJB_VALUE, val);
}
res = pushJsonbValue(state, WJB_END_OBJECT, NULL);
--
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] Safe memory allocation functions
Michael Paquier wrote Attached is a patch adding the following set of functions for frontend and backends returning NULL instead of reporting ERROR when allocation fails: - palloc_safe - palloc0_safe - repalloc_safe The only thing I can contribute is paint...I'm not fond of the word _safe and think _try would be more informative...in the spirit of try/catch as a means of error handling/recovery. David J. -- View this message in context: http://postgresql.nabble.com/Safe-memory-allocation-functions-tp5833709p5833711.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.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] New CF app deployment
Hi! Last I said something about the new CF app I said I was planning to deploy it over the holidays, and that clearly did not happen. But I've been talking to Michael, as the current cf-chief, and doing some further testing with it, and we think now is a good time to go for it :) As the plan is to close out the current CF just a few days from now, we're going to use that and try to swap it out when traffic is at least at it's lowest (even if we're well aware it's not going to be zero). So based on this, we plan to: In the late evening on Jan 19th (evening European time that is), I will make the current CF app readonly, and move it to a new url where it will remain available for the foreseeable future (we have a lot of useful data in it after all). Once this is done, Michael (primarily) will start working on syncing up the information about the latest patches into the new app. Much info is already synced there, but to make sure all the latest changes are included. In the morning European time on the 20th, I'll take over and try to finish up what's left. And sometime during the day when things are properly in sync, we'll open up the new app for business to all users. There are surely some bugs remaining in the system, so please have some oversight with that over the coming days/weeks. I'll try to get onto fixing them as soon as I can - and some others can look at that as well if it's something critical at least. Further status updates to come as we start working on it... -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Unused variables in hstore_to_jsonb
On Tue, Jan 13, 2015 at 4:34 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
Attached is a patch to fix that.
Oh, actually that's as well the case of hstore_to_jsonb_loose. Updated
patch is attached.
--
Michael
*** a/contrib/hstore/hstore_io.c
--- b/contrib/hstore/hstore_io.c
***
*** 1338,1344 hstore_to_jsonb(PG_FUNCTION_ARGS)
JsonbParseState *state = NULL;
JsonbValue *res;
! res = pushJsonbValue(state, WJB_BEGIN_OBJECT, NULL);
for (i = 0; i count; i++)
{
--- 1338,1344
JsonbParseState *state = NULL;
JsonbValue *res;
! pushJsonbValue(state, WJB_BEGIN_OBJECT, NULL);
for (i = 0; i count; i++)
{
***
*** 1349,1355 hstore_to_jsonb(PG_FUNCTION_ARGS)
key.val.string.len = HS_KEYLEN(entries, i);
key.val.string.val = HS_KEY(entries, base, i);
! res = pushJsonbValue(state, WJB_KEY, key);
if (HS_VALISNULL(entries, i))
{
--- 1349,1355
key.val.string.len = HS_KEYLEN(entries, i);
key.val.string.val = HS_KEY(entries, base, i);
! pushJsonbValue(state, WJB_KEY, key);
if (HS_VALISNULL(entries, i))
{
***
*** 1361,1367 hstore_to_jsonb(PG_FUNCTION_ARGS)
val.val.string.len = HS_VALLEN(entries, i);
val.val.string.val = HS_VAL(entries, base, i);
}
! res = pushJsonbValue(state, WJB_VALUE, val);
}
res = pushJsonbValue(state, WJB_END_OBJECT, NULL);
--- 1361,1367
val.val.string.len = HS_VALLEN(entries, i);
val.val.string.val = HS_VAL(entries, base, i);
}
! pushJsonbValue(state, WJB_VALUE, val);
}
res = pushJsonbValue(state, WJB_END_OBJECT, NULL);
***
*** 1385,1391 hstore_to_jsonb_loose(PG_FUNCTION_ARGS)
initStringInfo(tmp);
! res = pushJsonbValue(state, WJB_BEGIN_OBJECT, NULL);
for (i = 0; i count; i++)
{
--- 1385,1391
initStringInfo(tmp);
! pushJsonbValue(state, WJB_BEGIN_OBJECT, NULL);
for (i = 0; i count; i++)
{
***
*** 1396,1402 hstore_to_jsonb_loose(PG_FUNCTION_ARGS)
key.val.string.len = HS_KEYLEN(entries, i);
key.val.string.val = HS_KEY(entries, base, i);
! res = pushJsonbValue(state, WJB_KEY, key);
if (HS_VALISNULL(entries, i))
{
--- 1396,1402
key.val.string.len = HS_KEYLEN(entries, i);
key.val.string.val = HS_KEY(entries, base, i);
! pushJsonbValue(state, WJB_KEY, key);
if (HS_VALISNULL(entries, i))
{
***
*** 1471,1477 hstore_to_jsonb_loose(PG_FUNCTION_ARGS)
val.val.string.val = HS_VAL(entries, base, i);
}
}
! res = pushJsonbValue(state, WJB_VALUE, val);
}
res = pushJsonbValue(state, WJB_END_OBJECT, NULL);
--- 1471,1477
val.val.string.val = HS_VAL(entries, base, i);
}
}
! pushJsonbValue(state, WJB_VALUE, val);
}
res = pushJsonbValue(state, WJB_END_OBJECT, NULL);
--
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] Latches and barriers
Andres Freund and...@2ndquadrant.com writes: While it might not be required for existing latch uses (I'm *not* sure that's true), I still think that we should fix those XXX by actually using barriers now that we have them. I don't think we want every callsite worry about using barriers. Agreed? Yeah, now that we have barrier code we think works, we should definitely put some in there. The only reason it's like that is we didn't have any real barrier support at the time. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Latches and barriers
On Mon, Jan 12, 2015 at 11:27 AM, Andres Freund and...@2ndquadrant.com wrote: On 2015-01-12 11:03:42 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: While it might not be required for existing latch uses (I'm *not* sure that's true) I think at least syncrep.c might not be correct. In SyncRepWakeQueue() it sets PGPROC-syncRepState without the necessary barriers (via locks), although it does use them in SyncRepWaitForLSN(). It is, perhaps surprisingly to many, not sufficient to take a spinlock, change the flag, release it and then set the latch - the release alone doesn't guarantee a sufficient barrier unless looking at the flag is also protected by the spinlock. I thought we decided that a spinlock acquire or release should be a full barrier. -- 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] Latches and barriers
Andres Freund and...@2ndquadrant.com writes: On 2015-01-12 11:03:42 -0500, Tom Lane wrote: Yeah, now that we have barrier code we think works, we should definitely put some in there. The only reason it's like that is we didn't have any real barrier support at the time. Master only though? If we decide we need it earlier, we can backport that commit lateron... We've not been back-patching barrier fixes have we? I'm hesitant to put any of that stuff into released branches until it's had more time to age. 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] Latches and barriers
On 2015-01-12 12:44:56 -0500, Robert Haas wrote: On Mon, Jan 12, 2015 at 11:27 AM, Andres Freund and...@2ndquadrant.com wrote: On 2015-01-12 11:03:42 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: While it might not be required for existing latch uses (I'm *not* sure that's true) I think at least syncrep.c might not be correct. In SyncRepWakeQueue() it sets PGPROC-syncRepState without the necessary barriers (via locks), although it does use them in SyncRepWaitForLSN(). It is, perhaps surprisingly to many, not sufficient to take a spinlock, change the flag, release it and then set the latch - the release alone doesn't guarantee a sufficient barrier unless looking at the flag is also protected by the spinlock. I thought we decided that a spinlock acquire or release should be a full barrier. Acquire + release, yes. But not release alone? The x86 spinlock release currently is just a store - that won't ever be a full barrier. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Latches and barriers
On 2015-01-12 12:49:53 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2015-01-12 11:03:42 -0500, Tom Lane wrote: Yeah, now that we have barrier code we think works, we should definitely put some in there. The only reason it's like that is we didn't have any real barrier support at the time. Master only though? If we decide we need it earlier, we can backport that commit lateron... We've not been back-patching barrier fixes have we? Not fully at least. And I'm not sure that's good, because at least bgworker.c has relied on working barriers for a while and 9.4 introduced a couple more uses. I guess we should fix up the backbranch versions in a while. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] s_lock.h default definitions are rather confused
On 2015-01-10 23:03:36 +0100, Andres Freund wrote: On 2015-01-10 16:09:42 -0500, Tom Lane wrote: I've not tried to build HEAD on my HPPA dinosaur for awhile, but I did just now, and I am presented with boatloads of this: ../../../src/include/storage/s_lock.h:759: warning: `S_UNLOCK' redefined ../../../src/include/storage/s_lock.h:679: warning: this is the location of the previous definition which is not too surprising because the default definition at line 679 precedes the HPPA-specific one at line 759. That's 0709b7ee72e4bc71ad07b7120acd117265ab51d0. Not too surprising that it broke and wasn't noticed without access to hppa - the hppa code uses gcc inline assembly outside of the big defined(__GNUC__) and inside the section headed Platforms that use non-gcc inline assembly. I'm not particularly interested in untangling the effects of the recent hackery in s_lock.h enough to figure out how the overall structure got broken, but I trust one of you will clean up the mess. I think it's easiest solved by moving the gcc inline assembly up to the rest of the gcc inline assembly. That'll require duplicating a couple lines, but seems easier to understand nonetheless. Not pretty. Robert, do you have a better idea? Alternatively we could just add a #undef in the hppa gcc section and live with the uglyness. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] replicating DROP commands across servers
Jeff Janes wrote: On Fri, Jan 2, 2015 at 9:59 PM, Jeff Janes jeff.ja...@gmail.com wrote: This commit 3f88672a4e4d8e648d24ccc65 seems to have broken pg_upgrade for pg_trgm. It seems I failed to notice the get_object_address in the ALTER EXTENSION path. Should be fixed now. I looked for similar missed callsites and didn't find anything. Thanks for the report! -- Á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
