Re: [HACKERS] ThisTimeLineID in checkpointer and bgwriter processes
On 21.12.2012 08:18, Amit Kapila wrote:
On Thursday, December 20, 2012 11:15 PM Heikki Linnakangas wrote:
On 20.12.2012 18:19, Fujii Masao wrote:
InstallXLogFileSegment() also uses ThisTimeLineID. But your recent
commit
doesn't take care of it and prevents the standby from recycling the
WAL files
properly. Specifically, the standby recycles the WAL file to wrong
name.
A-ha, good catch. So that's actually a live bug in 9.1 and 9.2 as well:
after the recovery target timeline has changed, restartpoints will
continue to preallocate/recycle WAL files for the old timeline. That's
otherwise harmless, but the useless WAL files waste space, and
walreceiver will have to always create new files.
So instead of always running with ThisTimeLineID = 0 in the
checkpointer
process, I guess we'll have to update it to the timeline being
replayed,
when creating a restartpoint.
Shouldn't there be a check if(RecoveryInProgress), before assigning
RecoveryTargetTLI to ThisTimeLineID in CreateRestartPoint()?
Hmm, I don't think so. You're not supposed to get that far in
CreateRestartPoint() if recovery has already ended, or just being ended.
The startup process ends recovery, ie. makes RecoveryInProgress()
return false, only after writing the end-of-recovery checkpoint. And
after the end-of-recovery checkpoint has been written,
CreateRestartPoint() will do nothing, because the end-of-recovery
checkpoint is later than the last potential restartpoint. I'm talking
about this check in CreateRestartPoint():
if (XLogRecPtrIsInvalid(lastCheckPointRecPtr) ||
XLByteLE(lastCheckPoint.redo, ControlFile-checkPointCopy.redo))
{
ereport(DEBUG2,
(errmsg(skipping restartpoint, already performed at
%X/%X,
(uint32) (lastCheckPoint.redo
32), (uint32) lastCheckPoint.redo)));
...
return false;
}
However, there's this just before we recycle WAL segments:
/*
* Update pg_control, using current time. Check that it still shows
* IN_ARCHIVE_RECOVERY state and an older checkpoint, else do nothing;
* this is a quick hack to make sure nothing really bad happens if
somehow
* we get here after the end-of-recovery checkpoint.
*/
LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
if (ControlFile-state == DB_IN_ARCHIVE_RECOVERY
XLByteLT(ControlFile-checkPointCopy.redo, lastCheckPoint.redo))
{
...
but I believe that quick hack is just paranoia. You should not get
that far after the end-of-recovery checkpoint.
In any case, if you somehow get there anyway, the worst that will happen
is that some old WAL segments are recycled/preallocated on the old
timeline, wasting some space until the next checkpoint.
- 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] Review of Row Level Security
On 20 December 2012 19:42, Stephen Frost sfr...@snowman.net wrote: Kevin, all, * Kevin Grittner (kgri...@mail.com) wrote: The more secure behavior is to allow entry of data which will not be visible by the person doing the entry. wrt this- I'm inclined to agree with Kevin. It's certainly common in certain environments that you can write to a higher level than you can read from. Granting those writers access to read the data later would be... difficult. What we're really arguing about here, afaict, is what the default should be. In line with Kevin's comments and Tom's reading of the spec (along with my own experience in these environments), I'd argue for the default to allow writing rows you're not allowed to read. It would certainly be ideal if we could support both options, on a per-relation basis, when we release the overall feature. It doesn't feel like it'd be a lot of work to do that, but I've not been able to follow this discussion up til now. Thankfully, I'm hopeful that I'm going to have more time now to keep up with PG. :) It is certainly possible to deliver a row security feature that covers all the cases discussed in 9.3. I want that. 1. The case of row security applies to all commands is simple to implement and document, since it presents no anomalies. 2. As KaiGai has explained, there are significant anomalies in the behaviour of row security applies only to reads. Those anomalies need to be analysed carefully. They also need to be explained to the user in documentation. It's unreasonable for people to demand a feature yet provide no guidance to the person trying (hard) to provide that feature in a sensible way. If people genuinely believe case (2) is worth pursuing, additional work and input is needed so that KaiGai can make changes in time for the 9.3 deadline. Please read what KaiGai has said and respond. Since there are so many people reading this thread and wanting (2), that seems reasonable to expect. What I have proposed is that I work on the review for case (1) and then if we solve (2) that can go in also. I don't think its reasonable to reject the whole feature because of unresolved difficulties around one use case, which is what will happen if this is seen as merely a debate about defaults. -- Simon Riggs 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] Review of Row Level Security
On 21 December 2012 08:56, Simon Riggs si...@2ndquadrant.com wrote: It's unreasonable for people to demand a feature yet provide no guidance to the person trying (hard) to provide that feature in a sensible way. If people genuinely believe case (2) is worth pursuing, additional work and input is needed so that KaiGai can make changes in time for the 9.3 deadline. Please read what KaiGai has said and respond. Since there are so many people reading this thread and wanting (2), that seems reasonable to expect. What I have proposed is that I work on the review for case (1) and then if we solve (2) that can go in also. I don't think its reasonable to reject the whole feature because of unresolved difficulties around one use case, which is what will happen if this is seen as merely a debate about defaults. One comment on the code itself -- I think it needs some locking of rows from the subquery to ensure correct concurrency behaviour when there are multiple transactions doing updates at the same time. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review of Row Level Security
On 21 December 2012 09:29, Dean Rasheed dean.a.rash...@gmail.com wrote: On 21 December 2012 08:56, Simon Riggs si...@2ndquadrant.com wrote: It's unreasonable for people to demand a feature yet provide no guidance to the person trying (hard) to provide that feature in a sensible way. If people genuinely believe case (2) is worth pursuing, additional work and input is needed so that KaiGai can make changes in time for the 9.3 deadline. Please read what KaiGai has said and respond. Since there are so many people reading this thread and wanting (2), that seems reasonable to expect. What I have proposed is that I work on the review for case (1) and then if we solve (2) that can go in also. I don't think its reasonable to reject the whole feature because of unresolved difficulties around one use case, which is what will happen if this is seen as merely a debate about defaults. One comment on the code itself -- I think it needs some locking of rows from the subquery to ensure correct concurrency behaviour when there are multiple transactions doing updates at the same time. Another comment -- the use of the RangeTblEntry structure in the new code is a bit odd. It seems to be creating an RTE that is both an RTE_RELATION and an RTE_SUBQUERY at the same time. I think it would be cleaner to just add a new RTE for the subquery (cf. the trigger-updatable view code in ApplyRetrieveRule). Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] need a function to extract list items from pg_node_tree
In order to implement the PARAMETER_DEFAULT column in the information schema I need a way to get the expressions out of the proargdefaults column. pg_get_expr(proargdefaults, 0) gives me all expressions comma-separated, but I need them individually. I think a function like pg_get_list_nth (to keep consistent with the internal list API) could work. Alternatively, a list-to-array function might do the trick. Are there are any other potential uses cases that should be considered here? AFAICT, indexprs is the only other system catalog column that contains an expression list. -- 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] need a function to extract list items from pg_node_tree
On 2012-12-21 07:20:00 -0500, Peter Eisentraut wrote: In order to implement the PARAMETER_DEFAULT column in the information schema I need a way to get the expressions out of the proargdefaults column. pg_get_expr(proargdefaults, 0) gives me all expressions comma-separated, but I need them individually. I think a function like pg_get_list_nth (to keep consistent with the internal list API) could work. Alternatively, a list-to-array function might do the trick. Are there are any other potential uses cases that should be considered here? AFAICT, indexprs is the only other system catalog column that contains an expression list. Hm. Wouldn't it be better to create a pg_node_tree[] and use that in pg_attribute? Its already in the variable length part of pg_proc anyway... 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] pg_basebackup from cascading standby after timeline switch
On 17.12.2012 18:58, Magnus Hagander wrote:
On Mon, Dec 17, 2012 at 5:19 PM, Tom Lanet...@sss.pgh.pa.us wrote:
Heikki Linnakangashlinnakan...@vmware.com writes:
I'm not happy with the fact that we just ignore the problem in a backup
taken from a standby, silently giving the user a backup that won't start
up. Why not include the timeline history file in the backup?
+1. I was not aware that we weren't doing that --- it seems pretty
foolish, especially since as you say they're tiny.
Yeah, +1. That should probably have been a part of the whole
basebackup from slave patch, so it can probably be considered a
back-patchable bugfix in itself, no?
Yes, this should be backpatched to 9.2. I came up with the attached.
However, thinking about this some more, there's a another bug in the way
WAL files are included in the backup, when a timeline switch happens.
basebackup.c includes all the WAL files on ThisTimeLineID, but when the
backup is taken from a standby, the standby might've followed a timeline
switch. So it's possible that some of the WAL files should come from
timeline 1, while others should come from timeline 2. This leads to an
error like requested WAL segment 0001000C has already
been removed in pg_basebackup.
Attached is a script to reproduce that bug, if someone wants to play
with it. It's a bit sensitive to timing, and needs tweaking the paths at
the top.
One solution to that would be to pay more attention to the timelines to
include WAL from. basebackup.c could read the timeline history file, to
see exactly where the timeline switches happened, and then construct the
filename of each WAL segment using the correct timeline id. Another
approach would be to do readdir() on pg_xlog, and include all WAL files,
regardless of timeline IDs, that fall in the right XLogRecPtr range. The
latter seems easier to backpatch.
- Heikki
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index 65200c1..5c0deaa 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -12,12 +12,13 @@
*/
#include postgres.h
+#include string.h
#include sys/types.h
#include sys/stat.h
#include unistd.h
#include time.h
-#include access/xlog_internal.h /* for pg_start/stop_backup */
+#include access/xlog_internal.h
#include catalog/pg_type.h
#include lib/stringinfo.h
#include libpq/libpq.h
@@ -44,6 +45,7 @@ typedef struct
static int64 sendDir(char *path, int basepathlen, bool sizeonly);
+static void sendTimeLineHistoryFiles(void);
static void sendFile(char *readfilename, char *tarfilename,
struct stat * statbuf);
static void sendFileWithContent(const char *filename, const char *content);
@@ -286,6 +288,27 @@ perform_base_backup(basebackup_options *opt, DIR *tblspcdir)
break;
}
+ /*
+ * Include all timeline history files.
+ *
+ * The timeline history files are usually not strictly required to
+ * restore the backup, but if you take a backup from a standby server,
+ * and the WAL segment containing the checkpoint record contains WAL
+ * from an older timeline, recovery will complain on the older
+ * timeline's ID if there is no timeline history file listing it. This
+ * can happen if you take a backup right after promoting a standby to
+ * become new master, and take the backup from a different, cascading
+ * standby server.
+ *
+ * However, even when not strictly required, the timeline history
+ * files are tiny, and provide a lot of forensic information about the
+ * recovery history of the database, so it's best to always include
+ * them all. (If asked to include WAL, that is. Otherwise you need a
+ * WAL archive to restore anyway, and the timeline history files
+ * should be present in the archive)
+ */
+ sendTimeLineHistoryFiles();
+
/* Send CopyDone message for the last tar file */
pq_putemptymessage('c');
}
@@ -726,6 +749,58 @@ sendDir(char *path, int basepathlen, bool sizeonly)
return size;
}
+/*
+ * Include all timeline history files from pg_xlog in the output tar stream.
+ */
+static void
+sendTimeLineHistoryFiles(void)
+{
+ DIR *dir;
+ struct dirent *de;
+ char pathbuf[MAXPGPATH];
+ struct stat statbuf;
+
+ dir = AllocateDir(./pg_xlog);
+ while ((de = ReadDir(dir, ./pg_xlog)) != NULL)
+ {
+ CHECK_FOR_INTERRUPTS();
+
+ if (strlen(de-d_name) == 8 + strlen(.history)
+ strspn(de-d_name, 0123456789ABCDEF) == 8
+ strcmp(de-d_name + 8, .history) == 0)
+ {
+ /* It looks like a timeline history file. Include it. */
+ snprintf(pathbuf, MAXPGPATH, ./pg_xlog/%s, de-d_name);
+
+ if (lstat(pathbuf, statbuf) != 0)
+ {
+if (errno != ENOENT)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg(could not stat file or directory \%s\: %m,
+ pathbuf)));
+
+/* If the file went away while scanning, it's no error. */
+continue;
+ }
+
+ if (!S_ISREG(statbuf.st_mode))
+ {
+/*
+
Re: [HACKERS] need a function to extract list items from pg_node_tree
On 12/21/12 7:26 AM, Andres Freund wrote: On 2012-12-21 07:20:00 -0500, Peter Eisentraut wrote: In order to implement the PARAMETER_DEFAULT column in the information schema I need a way to get the expressions out of the proargdefaults column. pg_get_expr(proargdefaults, 0) gives me all expressions comma-separated, but I need them individually. I think a function like pg_get_list_nth (to keep consistent with the internal list API) could work. Alternatively, a list-to-array function might do the trick. Are there are any other potential uses cases that should be considered here? AFAICT, indexprs is the only other system catalog column that contains an expression list. Hm. Wouldn't it be better to create a pg_node_tree[] and use that in pg_attribute? Its already in the variable length part of pg_proc anyway... That sounds like a good idea. I don't know why they are currently stored as a list. -- 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] Cascading replication: should we detect/prevent cycles?
On Thu, Dec 20, 2012 at 5:28 PM, Joshua Berkus j...@agliodbs.com wrote: What would such a test look like? It's not obvious to me that there's any rapid way for a user to detect this situation, without checking each server individually. Change something on the master and observe that none of the supposed standbys notice? That doesn't sound like an infallible test, or a 60-second one. My point is that in a complex situation (imagine a shop with 9 replicated servers in 3 different cascaded groups, immediately after a failover of the original master), it would be easy for a sysadmin, responding to middle of the night page, to accidentally fat-finger an IP address and create a cycle instead of a new master. And once he's done that, a longish troubleshooting process to figure out what's wrong and why writes aren't working, especially if he goes to bed and some other sysadmin picks up the Writes failing to PostgreSQL ticket. *if* it's relatively easy for us to detect cycles (that's a big if, I'm not sure how we'd do it), then it would help a lot for us to at least emit a WARNING. That would short-cut a lot of troubleshooting. I'm sure it's possible; I don't *think* it's terribly easy. The usual algorithm for cycle detection is to have each node send to the next node the path that the data has taken. But, there's no unique identifier for each slave that I know of - you could use IP address, but that's not really unique. And, if the WAL passes through an archive, how do you deal with that? I'm sure somebody could figure all of this stuff out, but it seems fairly complicated for the benefit we'd get. I just don't think this is going to be a terribly common problem; if it turns out I'm wrong, I may revise my opinion. :-) To me, it seems that lag monitoring between master and standby is something that anyone running a complex replication configuration should be doing - and yeah, I think anything involving four standbys (or cascading) qualifies as complex. If you're doing that, you should notice pretty quickly that your replication lag is increasing steadily. You might also check pg_stat_replication the master and notice that there are no connections there any more. Could someone miss those tell-tale signs? Sure. But they could also set autovacuum_naptime to an hour and then file a support ticket complaining that about table bloat - and they do. Personally, as user screw-ups go, I'd consider that scenario (and its fourteen cousins, twenty-seven second cousins, and three hundred and ninety two other extended family members) as higher-priority and lower effort to fix than this particular thing. YMMV, of course. -- 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] Feature Request: pg_replication_master()
On Thu, Dec 20, 2012 at 5:07 PM, Joshua Berkus j...@agliodbs.com wrote: As ever, we spent much energy on debating backwards compatibility rather than just solving the problem it posed, which is fairly easy to solve. Well, IIRC, the debate was primarily of *your* making. Almost everyone else on the thread was fine with the original patch, and it was nearly done for 9.2 before you stepped in. I can't find anyone else on that thread who thought that backwards compatibility was more important than fixing the API. +1. Let's JFDI. -- 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] Review of Row Level Security
On Thu, Dec 20, 2012 at 4:50 PM, Kevin Grittner kgri...@mail.com wrote: I don't think I like ALTER TABLE as a syntax for row level security. How about using existing GRANT syntax but allowing a WHERE clause? That seems more natural to me, and it would make it easy to apply the same conditions to multiple types of operations when desired, but use different expressions when desired. Without having spent a lot of time pondering it, I think that if row level SELECT permissions exist, they would need to be met on the OLD tuple to allow DELETE or UPDATE, and UPDATE row level permissions would be applied to the NEW tuple. This gets thorny if a role inherits from multiple roles each having a different RLS predicate. You can OR them together, but performance will likely suck. I initially thought of this as well, but I think it's just too ugly to live. -- 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] Review of Row Level Security
On Fri, Dec 21, 2012 at 3:56 AM, Simon Riggs si...@2ndquadrant.com wrote: It's unreasonable for people to demand a feature yet provide no guidance to the person trying (hard) to provide that feature in a sensible way. You've got it backwards. All the issues that are being discussed now on this thread have been discussed previously on prior threads about row-level security. For the most part, nobody other than KaiGai and I participated in those, and we had a consensus hammered out that was reflected in the patch that started this thread. The person insisting on an eleventh-hour design change is you. -- 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] Feature Request: pg_replication_master()
On 20 December 2012 19:29, Bruce Momjian br...@momjian.us wrote: On Wed, Dec 19, 2012 at 10:34:14PM +, Simon Riggs wrote: On 19 December 2012 22:19, Joshua Berkus j...@agliodbs.com wrote: It stalled because the patch author decided not to implement the request to detect recovery.conf in data directory, which allows backwards compatibility. Well, I don't think we had agreement on how important backwards compatibility for recovery.conf was, particularly not on the whole recovery.conf/recovery.done functionality and the wierd formatting of recovery.conf. As ever, we spent much energy on debating backwards compatibility rather than just solving the problem it posed, which is fairly easy to solve. Let me also add that I am tired of having recovery.conf improvement stalled by backward compatibility concerns. At this point, let's just trash recovery.conf backward compatibility and move on. No, lets not. The only stall happening is because of a refusal to listen to another person's reasonable request during patch review. That requirement is not a blocker to the idea, it just needs to be programmed. Lets just implement the reasonable request for backwards compatibility, rather than wasting time on reopening the debate. -- Simon Riggs 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] Feature Request: pg_replication_master()
On 20 December 2012 19:29, Bruce Momjian br...@momjian.us wrote: At this point backward compatibility has paralized us from fixing a recovery.conf API that everyone agrees is non-optimal, and this has gone on for multiple major releases. I don't care what we have to do, just clean this up for 9.3! The main stall at this point is that the developer who wrote that patch no longer spends much time working on Postgres. AFAICS there is nobody working on this for 9.3 mainly because its not a priority, nor will implementing that fix the OP's request. There is no paralysis because there never was a blocker, only a request for backwards compatibility, which is easily possible to implement. -- Simon Riggs 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] Review of Row Level Security
On 21 December 2012 14:19, Robert Haas robertmh...@gmail.com wrote: On Fri, Dec 21, 2012 at 3:56 AM, Simon Riggs si...@2ndquadrant.com wrote: It's unreasonable for people to demand a feature yet provide no guidance to the person trying (hard) to provide that feature in a sensible way. You've got it backwards. All the issues that are being discussed now on this thread have been discussed previously on prior threads about row-level security. For the most part, nobody other than KaiGai and I participated in those, and we had a consensus hammered out that was reflected in the patch that started this thread. The person insisting on an eleventh-hour design change is you. Forwards or backwards, the problems of that previous design still exist and need some attention before we can commit. If you can spend some time investigating the problems and documenting how it works in that mode, it would be a great help for this patch. -- Simon Riggs 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] Event Triggers: adding information
On Wed, Dec 12, 2012 at 4:47 PM, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: The previously attached patch already needs a rebase since Tom fixed the single user mode where you're not supposed to access potentially corrupted system indexes. Please find attached a new version of the patch that should applies cleanly to master (I came to trust git). Sorry for the delay - I'm looking at this now. My first observation (a.k.a. gripe) is this patch touches an awful lot of code all over the backend. We just did a big old refactoring to try to get all the rename and alter owner commands to go through the same code path, but if this patch is any indication it has not done us a whole lot of good. The whole idea of all that work is that when people wanted to make systematic changes to those operations (like involving sepgsql, or returning the OID) there would not be 47 places to change. Apparently, we aren't there yet. Maybe we need some more refactoring of that stuff before tackling this? Does anyone object to the idea of making every command that creates a new SQL object return the OID of an object, and similarly for RENAME / ALTER TO? If so, maybe we ought to go through and do those things first, as separate patches, and then rebase this over those changes for simplicity. I can probably do that real soon now, based on this patch, if there are no objections, but I don't want to do the work and then have someone say, ack, what have you done? I'm basically implacably opposed to the ddl_rewrite.c part of this patch. I think that the burden of maintaining reverse-parsing code for all the DDL we support is an unacceptable level of maintenance overhead for this feature. It essentially means that every future change to gram.y that affects any of the supported commands will require a compensating change to ddl_rewrite.c. That is a whole lot of work and it is almost guaranteed that future patch authors will sometimes fail to do it correctly. At an absolute bare minimum I think we would need some kind of comprehensive test suite for this functionality, as opposed to the less than 60 lines of new test cases this patch adds which completely fail to test this code in any way at all, but really I think we should just not have it. It WILL get broken (if it isn't already) and it WILL slow down future development of SQL features. It also WILL have edge cases where it doesn't work properly, such as where the decision of the actual index name to use is only decided at execution time and cannot be inferred from the parse tree. I know that you feel that all of these are tolerable costs, but I 100% disgaree. I predict that if we commit this, it will becomes a never-ending and irrecoverable font of trouble. -- 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] Review of Row Level Security
Kohei KaiGai wrote: I don't think I like ALTER TABLE as a syntax for row level security. How about using existing GRANT syntax but allowing a WHERE clause? That seems more natural to me, and it would make it easy to apply the same conditions to multiple types of operations when desired, but use different expressions when desired. It seems to me this syntax gives an impression that row-security feature is tightly combined with role-mechanism, even though it does not need. For example, we can set row-security policy of primary key is even/odd number, independent from working role. Is there some high-level discussion of the concept of row level security that operates independently of roles? I'm having a little trouble getting my head around the idea, there is no README in the patch, and the Wiki page on RLS hasn't been updated for two and a half years and seems to be mainly discussing the possibility of adding non-leaky views (which we now have). If it doesn't control which roles have access to which rows, what does it do, conceptually? (A URL to a page explaining this would be fine.) -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] PL/PgSQL STRICT
Hi,
Courtesy of me, Christmas comes a bit early this year. I wrote a patch
which allows you to add STRICT into PERFORM and INSERT/UPDATE/DELETE
without specifying an INTO clause. Observe:
=# create table foo(a int);
CREATE TABLE
=# create function foof() returns void as $$ begin update strict foo set
a=a+1; end $$ language plpgsql;
CREATE FUNCTION
=# select foof();
ERROR: query returned no rows
I know everyone obviously wants this, so I will be sending another
version with regression tests and documentation later. The code is a
bit ugly at places, but I'm going to work on that too.
Regards,
Marko Tiikkaja
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
***
*** 1500,1508 static int
exec_stmt_perform(PLpgSQL_execstate *estate, PLpgSQL_stmt_perform *stmt)
{
PLpgSQL_expr *expr = stmt-expr;
(void) exec_run_select(estate, expr, 0, NULL);
! exec_set_found(estate, (estate-eval_processed != 0));
exec_eval_cleanup(estate);
return PLPGSQL_RC_OK;
--- 1500,1519
exec_stmt_perform(PLpgSQL_execstate *estate, PLpgSQL_stmt_perform *stmt)
{
PLpgSQL_expr *expr = stmt-expr;
+ uint32 n;
(void) exec_run_select(estate, expr, 0, NULL);
! n = estate-eval_processed;
! if (stmt-strict n == 0)
! ereport(ERROR,
! (errcode(ERRCODE_NO_DATA_FOUND),
!errmsg(query returned no rows)));
! else if (stmt-strict n 1)
! ereport(ERROR,
! (errcode(ERRCODE_TOO_MANY_ROWS),
!errmsg(query returned more than one row)));
!
! exec_set_found(estate, (n != 0));
exec_eval_cleanup(estate);
return PLPGSQL_RC_OK;
***
*** 3211,3217 exec_stmt_execsql(PLpgSQL_execstate *estate,
* forcing completion of a sequential scan. So don't do it unless we
need
* to enforce strictness.
*/
! if (stmt-into)
{
if (stmt-strict || stmt-mod_stmt)
tcount = 2;
--- 3222,3228
* forcing completion of a sequential scan. So don't do it unless we
need
* to enforce strictness.
*/
! if (stmt-into || stmt-strict)
{
if (stmt-strict || stmt-mod_stmt)
tcount = 2;
***
*** 3335,3340 exec_stmt_execsql(PLpgSQL_execstate *estate,
--- 3346,3366
exec_eval_cleanup(estate);
SPI_freetuptable(SPI_tuptable);
}
+ else if (stmt-strict)
+ {
+ /*
+* If a mod stmt specified STRICT, and the query didn't find
+* exactly one row, throw an error.
+*/
+ if (SPI_processed == 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_NO_DATA_FOUND),
+errmsg(query returned no rows)));
+ else if (SPI_processed 1)
+ ereport(ERROR,
+ (errcode(ERRCODE_TOO_MANY_ROWS),
+errmsg(query returned more than one
row)));
+ }
else
{
/* If the statement returned a tuple table, complain */
*** a/src/pl/plpgsql/src/pl_funcs.c
--- b/src/pl/plpgsql/src/pl_funcs.c
***
*** 1187,1193 static void
dump_perform(PLpgSQL_stmt_perform *stmt)
{
dump_ind();
! printf(PERFORM expr = );
dump_expr(stmt-expr);
printf(\n);
}
--- 1187,1193
dump_perform(PLpgSQL_stmt_perform *stmt)
{
dump_ind();
! printf(PERFORM%s expr = , stmt-strict ? STRICT : );
dump_expr(stmt-expr);
printf(\n);
}
*** a/src/pl/plpgsql/src/pl_gram.y
--- b/src/pl/plpgsql/src/pl_gram.y
***
*** 178,183 static List*read_raise_options(void);
--- 178,184
%type expr expr_until_semi expr_until_rightbracket
%type expr expr_until_then expr_until_loop opt_expr_until_when
%type expr opt_exitcond
+ %type boolean opt_strict
%type ival assign_var foreach_slice
%type var cursor_variable
***
*** 834,847 proc_stmt : pl_block ';'
{ $$ = $1; }
;
! stmt_perform : K_PERFORM expr_until_semi
{
PLpgSQL_stmt_perform *new;
new =
palloc0(sizeof(PLpgSQL_stmt_perform));
new-cmd_type =
PLPGSQL_STMT_PERFORM;
new-lineno =
plpgsql_location_to_lineno(@1);
! new-expr = $2;
Re: [HACKERS] PL/PgSQL STRICT
Marko Tiikkaja pgm...@joh.to writes: Courtesy of me, Christmas comes a bit early this year. I wrote a patch which allows you to add STRICT into PERFORM and INSERT/UPDATE/DELETE without specifying an INTO clause. What is the use-case for this? Won't this result in the word STRICT becoming effectively reserved in contexts where it currently is not? (IOW, even if the feature is useful, I've got considerable doubts about this syntax for it. The INTO clause is an ugly, badly designed kluge already --- let's not make another one just like it.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review of Row Level Security
On 21 December 2012 14:48, Kevin Grittner kgri...@mail.com wrote:
Kohei KaiGai wrote:
I don't think I like ALTER TABLE as a syntax for row level
security. How about using existing GRANT syntax but allowing a
WHERE clause? That seems more natural to me, and it would make
it easy to apply the same conditions to multiple types of
operations when desired, but use different expressions when
desired.
It seems to me this syntax gives an impression that row-security
feature is tightly combined with role-mechanism, even though it
does not need. For example, we can set row-security policy of
primary key is even/odd number, independent from working role.
Is there some high-level discussion of the concept of row level
security that operates independently of roles? I'm having a little
trouble getting my head around the idea, there is no README in the
patch, and the Wiki page on RLS hasn't been updated for two and a
half years and seems to be mainly discussing the possibility of
adding non-leaky views (which we now have). If it doesn't control
which roles have access to which rows, what does it do,
conceptually? (A URL to a page explaining this would be fine.)
There's some docs there, but that needs improving.
Each table has a single security clause. The clause doesn't enforce
that it must contain something that depends on role, but that is the
most easily understood usage of it. We do that to ensure that you can
embed the intelligence into a function, say hasRowLevelAccess(), which
doesn't have any provable relationship on role, and maybe wouldn't do
anything in unit test, yet clearly in production it would do so.
It would be easy enough to include read/write variable clauses by
using a function similar to TG_OP for triggers, e.g. StatementType.
That would make the security clause that applied only to reads into
something like this (StatementType('INSERT, UPDATE') OR other-quals).
If you push for GRANT ... WHERE then you may as well just say you want
the patch cancelled in this release. There's no way to analyze, design
and code something like that in 3 weeks. As I've said, I single
table-level policy is much easier to manage anyway.
--
Simon Riggs 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] PL/PgSQL STRICT
On 12/21/12 4:39 PM, Tom Lane wrote: What is the use-case for this? Currently, the way to do this would be something like: DECLARE _ok bool; BEGIN UPDATE foo .. RETURNING TRUE INTO STRICT _ok; We have a lot of code like this, and I bet others do as well. Won't this result in the word STRICT becoming effectively reserved in contexts where it currently is not? It will, which probably is not ideal if it can be avoided. I also considered syntax like INTO STRICT NULL, but that felt a bit ugly. It would be great if someone had any smart ideas about the syntax. Regards, Marko Tiikkaja -- 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 STRICT
On 12/21/12 4:49 PM, I wrote: On 12/21/12 4:39 PM, Tom Lane wrote: What is the use-case for this? Currently, the way to do this would be something like: I realize I didn't really answer the question. The use case is when you're UPDATEing or DELETEing a row and you want to quickly assert that there should be exactly one row. For example, if you've previously locked a row with SELECT .. FOR UPDATE, and now you want to UPDATE or DELETE it, it better be there (or you have a bug somewhere). Regards, Marko Tiikkaja -- 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] need a function to extract list items from pg_node_tree
Peter Eisentraut pete...@gmx.net writes: On 12/21/12 7:26 AM, Andres Freund wrote: Hm. Wouldn't it be better to create a pg_node_tree[] and use that in pg_attribute? Its already in the variable length part of pg_proc anyway... That sounds like a good idea. I don't know why they are currently stored as a list. They're stored as a list because that's what's convenient for use by the parser/planner. I believe that a change like this would be quite inconvenient on that end, and that that is not where we want to put the inconvenience. I'm also concerned about possibly breaking any third-party code that's already coping with the current representation. 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] PL/PgSQL STRICT
On 12/21/12 4:49 PM, I wrote: Won't this result in the word STRICT becoming effectively reserved in contexts where it currently is not? It will, which probably is not ideal if it can be avoided. I also considered syntax like INTO STRICT NULL, but that felt a bit ugly. It would be great if someone had any smart ideas about the syntax. Another idea would be to force the STRICT to be immediately after INSERT, UPDATE or DELETE. Regards, Marko Tiikkaja -- 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 STRICT
2012/12/21 Marko Tiikkaja pgm...@joh.to: On 12/21/12 4:49 PM, I wrote: On 12/21/12 4:39 PM, Tom Lane wrote: What is the use-case for this? Currently, the way to do this would be something like: I realize I didn't really answer the question. The use case is when you're UPDATEing or DELETEing a row and you want to quickly assert that there should be exactly one row. For example, if you've previously locked a row with SELECT .. FOR UPDATE, and now you want to UPDATE or DELETE it, it better be there (or you have a bug somewhere). yes, it has sense probably only after keyword it should be simple implementable Regards Pavel Regards, Marko Tiikkaja -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] pgcrypto seeding problem when ssl=on
When there is 'ssl=on' then postmaster calls SSL_CTX_new(), which asks for random number, thus requiring initialization of randomness pool (RAND_poll). After that all forked backends think pool is already initialized. Thus they proceed with same fixed state they got from postmaster. Output is not completely constant due to: 1) When outputting random bytes, OpenSSL includes getpid() output when hashing pool state. 2) SSL_accept() adds time() output into pool before asking for random bytes. This and 1) should make sure SSL handshake is done with unique random numbers. [It's uncertain tho' how unpredictable the PRNG is in such mode.] Now, problem in pgcrypto side is that when compiled with OpenSSL, it expects the randomness pool to be already initialized. This expectation is filled when ssl=off, or when actual SSL connection is used. But it's broken when non-SSL connection is used while having ssl=on in config. Then all backends are in same state when they reach pgcrypto functions first time and output is only affected by pid. Affected: * pgp_encrypt*() - it feeds hashes of user data back to pool, but this is randomized, thus there is small chance first few messages have weaker keys. * gen_random_bytes() - this does not feed back anything, thus when used alone in session, it's output will repeat quite easily as randomness sequence is affected only by pid. Attached patch makes both gen_random_bytes() and pgp_encrypt() seed pool with output from gettimeofday(), thus getting pool off from fixed state. Basically, this mirrors what SSL_accept() already does. I'd like to do bigger reorg of seeding code in pgcrypto, but that might not be back-patchable. So I propose this simple fix, which should be applied also to older releases. -- marko pgcrypto-add-time.diff 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] PL/PgSQL STRICT
On Fri, Dec 21, 2012 at 10:39 AM, Tom Lane t...@sss.pgh.pa.us wrote: Marko Tiikkaja pgm...@joh.to writes: Courtesy of me, Christmas comes a bit early this year. I wrote a patch which allows you to add STRICT into PERFORM and INSERT/UPDATE/DELETE without specifying an INTO clause. What is the use-case for this? Won't this result in the word STRICT becoming effectively reserved in contexts where it currently is not? (IOW, even if the feature is useful, I've got considerable doubts about this syntax for it. The INTO clause is an ugly, badly designed kluge already --- let's not make another one just like it.) Yep, the use case for this seems mighty narrow to me. I could use GET DIAGNOSTICS to determine if nothing got altered, and it seems likely to me that expressly doing this via IF/ELSE/END IF would be easier to read in function code than a somewhat magic STRICT side-effect. I certainly appreciate that brevity can make things more readable, it's just that I'm not sure that is much of a help here. This is adding specific syntax for what seems like an unusual case to me, which seems like an unworthwhile complication. -- When confronted by a difficult problem, solve it by reducing it to the question, How would the Lone Ranger handle this?
Re: [HACKERS] PL/PgSQL STRICT
-Original Message- From: pgsql-hackers-ow...@postgresql.org [mailto:pgsql-hackers- ow...@postgresql.org] On Behalf Of Marko Tiikkaja Sent: Friday, December 21, 2012 10:53 AM To: Tom Lane Cc: PostgreSQL-development Subject: Re: [HACKERS] PL/PgSQL STRICT On 12/21/12 4:49 PM, I wrote: On 12/21/12 4:39 PM, Tom Lane wrote: What is the use-case for this? Currently, the way to do this would be something like: I realize I didn't really answer the question. The use case is when you're UPDATEing or DELETEing a row and you want to quickly assert that there should be exactly one row. For example, if you've previously locked a row with SELECT .. FOR UPDATE, and now you want to UPDATE or DELETE it, it better be there (or you have a bug somewhere). There had better be exactly one row - but who cares whether that is the row we were actually expecting to delete/update... I've recently had the experience of missing a WHERE pk = ... clause in an UPDATE statement inside a function so I do see the value in having an easy to implement safety idiom along these lines. Along the lines of EXPLAIN (options) CMD would something like UPDATE|DELETE (STRICT) identifier work? David J. -- 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 STRICT
Marko Tiikkaja pgm...@joh.to writes: Another idea would be to force the STRICT to be immediately after INSERT, UPDATE or DELETE. What about before it, ie STRICT UPDATE ... This should dodge the problem of possible conflict with table names, and it seems to me to read more naturally too. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Writing Trigger Functions in C
Hello guys, I've been finding performance issues when using a trigger to modify inserts on a partitioned table. If using the trigger the total time goes from 1 Hour to 4 hours. The trigger is pretty simple: CREATE OR REPLACE FUNCTION quotes_insert_trigger() RETURNS trigger AS $$ BEGIN EXECUTE 'INSERT INTO quotes_'|| to_char(new.received_time,'_MM_DD') ||' VALUES (($1).*)' USING NEW ; RETURN NULL; END; $$ LANGUAGE plpgsql; I've seen that some of you guys have worked on writing triggers in C. Does anyone have had an experience writing a trigger for partitioning in C ? If you have some code to paste so I can start from I will really appreciate. Thanks -- 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 STRICT
2012/12/21 Tom Lane t...@sss.pgh.pa.us: Marko Tiikkaja pgm...@joh.to writes: Another idea would be to force the STRICT to be immediately after INSERT, UPDATE or DELETE. What about before it, ie STRICT UPDATE ... This should dodge the problem of possible conflict with table names, and it seems to me to read more naturally too. +1 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 -- 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 STRICT
On 12/21/12 5:09 PM, Christopher Browne wrote: I could use GET DIAGNOSTICS to determine if nothing got altered, and it seems likely to me that expressly doing this via IF/ELSE/END IF would be easier to read in function code than a somewhat magic STRICT side-effect. STRICT is used in INTO, so PL/PgSQL users should already have an idea what it's going to do outside of INTO. I certainly appreciate that brevity can make things more readable, it's just that I'm not sure that is much of a help here. This is adding specific syntax for what seems like an unusual case to me, which seems like an unworthwhile complication. A quick grep suggests that our (the company I work for) code base has 160 occurrences of INSERT/UPDATE/DELETE followed by IF NOT FOUND THEN RAISE EXCEPTION. So it doesn't seem like an unusual case to me. Of course, some of them couldn't use STRICT because they are expected to happen (in which case they can send a more descriptive error message), but most of them could. Regards, Marko Tiikkaja -- 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 STRICT
On 12/21/12 5:22 PM, Tom Lane wrote: Marko Tiikkaja pgm...@joh.to writes: Another idea would be to force the STRICT to be immediately after INSERT, UPDATE or DELETE. What about before it, ie STRICT UPDATE ... This should dodge the problem of possible conflict with table names, and it seems to me to read more naturally too. Yeah, putting STRICT after the command wouldn't work for UPDATE. I like this one best so far, so I'm going with this syntax for the next version of the patch. Regards, Marko Tiikkaja -- 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] Event Triggers: adding information
Hi, Robert Haas robertmh...@gmail.com writes: Sorry for the delay - I'm looking at this now. Thanks very much! My first observation (a.k.a. gripe) is this patch touches an awful lot of code all over the backend. We just did a big old refactoring to try to get all the rename and alter owner commands to go through the same code path, but if this patch is any indication it has not done us a whole lot of good. The whole idea of all that work is that when people wanted to make systematic changes to those operations (like involving sepgsql, or returning the OID) there would not be 47 places to change. Apparently, we aren't there yet. Maybe we need some more refactoring of that stuff before tackling this? It's hard to devise exactly what kind of refactoring we need to do before trying to write a patch that benefits from it, in my experience. In some cases the refactoring will make things more complex, not less. Does anyone object to the idea of making every command that creates a new SQL object return the OID of an object, and similarly for RENAME / ALTER TO? If so, maybe we ought to go through and do those things first, as separate patches, and then rebase this over those changes for simplicity. I can probably do that real soon now, based on this patch, if there are no objections, but I don't want to do the work and then have someone say, ack, what have you done? That's a good idea. I only started quite late in that patch to work on the ObjectID piece of information, that's why I didn't split it before doing so. We might want to commit the other parts of the new infrastructure and information first then get back on ObjectID and command string, what do you think? Do you want me to prepare a patch that way, or would you rather hack your way around the ObjectID APIs then I would rebase the current patch? Also, keep in mind we want the ObjectID in all CREATE, ALTER and DROP statements, so my current patch is still some bricks shy of a load… (I added ObjectID only in the commands I added rewrite support for, apart from DROP). I'm basically implacably opposed to the ddl_rewrite.c part of this patch. I think that the burden of maintaining reverse-parsing code for all the DDL we support is an unacceptable level of maintenance overhead for this feature. It essentially means that every future I hear you. That feature is still required for any automatic consumption of the command string because you want to resolve schema names, index and constraint names, type name lookups and some more. I think that this feature is also not an option for in-core logical replication to support DDL at all. What alternative do you propose? change to gram.y that affects any of the supported commands will require a compensating change to ddl_rewrite.c. That is a whole lot of work and it is almost guaranteed that future patch authors will sometimes fail to do it correctly. At an absolute bare minimum I think we would need some kind of comprehensive test suite for this functionality, as opposed to the less than 60 lines of new test cases this patch adds which completely fail to test this code in any way at all, but really I think we should just not have it. It WILL get I had a more complete test suite last rounds and you did oppose to it for good reasons. I'm ok to revisit that now, and I think the test case should look like this: - install an auditing command trigger that will insert any DDL command string into a table with a sequence ordering - select * from audit - \d… of all objects created - drop schema cascade - DO $$ loop for sql in select command from audit do execute sql … - \d… of all objects created again Then any whacking of the grammar should alter the output here and any case that's not supported will fail too. We might be able to have a better way of testing the feature here, but I tried to stay into the realms of what I know pg_regress able to do. broken (if it isn't already) and it WILL slow down future development of SQL features. It also WILL have edge cases where it doesn't work properly, such as where the decision of the actual index name to use is only decided at execution time and cannot be inferred from the parse tree. I know that you feel that all of these are tolerable The way to solve that, I think, as Tom said, is to only rewrite the command string when the objects exist in the catalogs. That's why we now have ddl_command_trace which is an alias to either start or end depending on whether we're doing a DROP or a CREATE or ALTER operation. costs, but I 100% disgaree. I predict that if we commit this, it will becomes a never-ending and irrecoverable font of trouble. I'm not saying the cost of doing things that way are easy to swallow, I'm saying that I don't see any other way to get that feature reliably enough for in-core logical replication to just work with DDLs. Do you? Regards, -- Dimitri Fontaine http://2ndQuadrant.fr
Re: [HACKERS] Writing Trigger Functions in C
On Fri, Dec 21, 2012 at 10:25 AM, Charles Gomes charle...@outlook.com wrote: Hello guys, I've been finding performance issues when using a trigger to modify inserts on a partitioned table. If using the trigger the total time goes from 1 Hour to 4 hours. The trigger is pretty simple: CREATE OR REPLACE FUNCTION quotes_insert_trigger() RETURNS trigger AS $$ BEGIN EXECUTE 'INSERT INTO quotes_'|| to_char(new.received_time,'_MM_DD') ||' VALUES (($1).*)' USING NEW ; RETURN NULL; END; $$ LANGUAGE plpgsql; I've seen that some of you guys have worked on writing triggers in C. Does anyone have had an experience writing a trigger for partitioning in C ? If you have some code to paste so I can start from I will really appreciate. Honestly I'd leave the trigger alone and modify the client code in performance sensitive places to insert directly to the correct partition table. merlin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Writing Trigger Functions in C
Hello you can find lot of examples in PostgreSQL source code - see postgresql/contrib/spi directory and documentation http://www.postgresql.org/docs/9.0/static/trigger-example.html Regards Pavel Stehule 2012/12/21 Charles Gomes charle...@outlook.com: Hello guys, I've been finding performance issues when using a trigger to modify inserts on a partitioned table. If using the trigger the total time goes from 1 Hour to 4 hours. The trigger is pretty simple: CREATE OR REPLACE FUNCTION quotes_insert_trigger() RETURNS trigger AS $$ BEGIN EXECUTE 'INSERT INTO quotes_'|| to_char(new.received_time,'_MM_DD') ||' VALUES (($1).*)' USING NEW ; RETURN NULL; END; $$ LANGUAGE plpgsql; I've seen that some of you guys have worked on writing triggers in C. Does anyone have had an experience writing a trigger for partitioning in C ? If you have some code to paste so I can start from I will really appreciate. Thanks -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- 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 STRICT
On Fri, Dec 21, 2012 at 4:22 PM, Tom Lane t...@sss.pgh.pa.us wrote: What about before it, ie STRICT UPDATE ... +1 from me too. This feature would be awesome. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review of Row Level Security
Simon Riggs wrote:
Each table has a single security clause. The clause doesn't enforce
that it must contain something that depends on role, but that is the
most easily understood usage of it. We do that to ensure that you can
embed the intelligence into a function, say hasRowLevelAccess(), which
doesn't have any provable relationship on role, and maybe wouldn't do
anything in unit test, yet clearly in production it would do so.
It would be easy enough to include read/write variable clauses by
using a function similar to TG_OP for triggers, e.g. StatementType.
That would make the security clause that applied only to reads into
something like this (StatementType('INSERT, UPDATE') OR other-quals).
In the case where the logic in encapsulated into a function, what
are the logical differences from a BEFORE EACH ROW trigger? If
none, and this is strictly an optimization, what are the benchmarks
showing?
-Kevin
--
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 STRICT
Christopher Browne cbbro...@gmail.com writes: This is adding specific syntax for what seems like an unusual case to me, which seems like an unworthwhile complication. That was my first reaction too, but Marko's followon examples seem to make a reasonable case for it. There are many situations where you expect an UPDATE or DELETE to hit exactly one row. Often, programmers won't bother to add code to check that it did ... but if a one-word addition to the command can provide such a check, it seems more likely that they would add the check. 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] Writing Trigger Functions in C
On 12/21/2012 08:39 AM, Merlin Moncure wrote: On Fri, Dec 21, 2012 at 10:25 AM, Charles Gomes charle...@outlook.com wrote: Hello guys, I've been finding performance issues when using a trigger to modify inserts on a partitioned table. If using the trigger the total time goes from 1 Hour to 4 hours. The trigger is pretty simple: CREATE OR REPLACE FUNCTION quotes_insert_trigger() RETURNS trigger AS $$ BEGIN EXECUTE 'INSERT INTO quotes_'|| to_char(new.received_time,'_MM_DD') ||' VALUES (($1).*)' USING NEW ; RETURN NULL; END; $$ LANGUAGE plpgsql; I've seen that some of you guys have worked on writing triggers in C. Does anyone have had an experience writing a trigger for partitioning in C ? If you have some code to paste so I can start from I will really appreciate. Honestly I'd leave the trigger alone and modify the client code in performance sensitive places to insert directly to the correct partition table. I second that recommendation -- your performance will be much, much, better. Joe -- Joe Conway credativ LLC: http://www.credativ.us Linux, PostgreSQL, and general Open Source Training, Service, Consulting, 24x7 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] Writing Trigger Functions in C
On Fri, Dec 21, 2012 at 11:25 AM, Charles Gomes charle...@outlook.com wrote: Hello guys, I've been finding performance issues when using a trigger to modify inserts on a partitioned table. If using the trigger the total time goes from 1 Hour to 4 hours. The trigger is pretty simple: CREATE OR REPLACE FUNCTION quotes_insert_trigger() RETURNS trigger AS $ BEGIN EXECUTE 'INSERT INTO quotes_'|| to_char(new.received_time,'_MM_DD') ||' VALUES (($1).*)' USING NEW ; RETURN NULL; END; $ LANGUAGE plpgsql; I've seen that some of you guys have worked on writing triggers in C. Does anyone have had an experience writing a trigger for partitioning in C ? I'd want to be very careful about assuming that implementing the trigger function in C would necessarily improve performance. It's pretty likely that it wouldn't help much, as a fair bit of the cost of firing a trigger have to do with figuring out which function to call, marshalling arguments, and calling the function, none of which would magically disappear by virtue of implementing in C. A *major* cost that your existing implementation has is that it's re-planning the queries for every single invocation. This is an old, old problem from the Lisp days, EVAL considered evil http://stackoverflow.com/questions/2571401/why-exactly-is-eval-evil The EXECUTE winds up replanning queries every time the trigger fires. If you can instead enumerate the partitions explicitly, putting them into (say) a CASE clause, the planner could generate the plan once, rather than a million times, which would be a HUGE savings, vastly greater than you could expect from recoding into C. The function might look more like: create or replace function quotes_insert_trigger () returns trigger as $$ declare c_rt text; begin c_rt := to_char(new.received_time, '_MM_DD'); case c_rt when '2012_03_01' then insert into 2012_03_01 values (NEW.*) using new; when '2012_03_02' then insert into 2012_03_02 values (NEW.*) using new; else raise exception 'Need a new partition function for %', c_rt; end case; end $$ language plpgsql; You'd periodically need to change the function to reflect the existing set of partitions, but that's cheaper than creating a new partition. The case statement gets more expensive (in effect O(n) on the number of partitions, n) as the number of partitions increases. You could split the date into pieces (e.g. - years, months, days) to diminish that cost. But at any rate, this should be *way* faster than what you're running now, and not at any heinous change in development costs (as would likely be the case reimplementing using SPI). -- When confronted by a difficult problem, solve it by reducing it to the question, How would the Lone Ranger handle this?
Re: [HACKERS] PL/PgSQL STRICT
2012/12/21 Tom Lane t...@sss.pgh.pa.us: Christopher Browne cbbro...@gmail.com writes: This is adding specific syntax for what seems like an unusual case to me, which seems like an unworthwhile complication. That was my first reaction too, but Marko's followon examples seem to make a reasonable case for it. There are many situations where you expect an UPDATE or DELETE to hit exactly one row. Often, programmers won't bother to add code to check that it did ... but if a one-word addition to the command can provide such a check, it seems more likely that they would add the check. and it can be used for optimization - it can be optimized for fast first row 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 -- 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] Writing Trigger Functions in C
2012/12/21 Joe Conway m...@joeconway.com: On 12/21/2012 08:39 AM, Merlin Moncure wrote: On Fri, Dec 21, 2012 at 10:25 AM, Charles Gomes charle...@outlook.com wrote: Hello guys, I've been finding performance issues when using a trigger to modify inserts on a partitioned table. If using the trigger the total time goes from 1 Hour to 4 hours. The trigger is pretty simple: CREATE OR REPLACE FUNCTION quotes_insert_trigger() RETURNS trigger AS $$ BEGIN EXECUTE 'INSERT INTO quotes_'|| to_char(new.received_time,'_MM_DD') ||' VALUES (($1).*)' USING NEW ; RETURN NULL; END; $$ LANGUAGE plpgsql; I've seen that some of you guys have worked on writing triggers in C. Does anyone have had an experience writing a trigger for partitioning in C ? If you have some code to paste so I can start from I will really appreciate. Honestly I'd leave the trigger alone and modify the client code in performance sensitive places to insert directly to the correct partition table. I second that recommendation -- your performance will be much, much, better. sure Pavel Joe -- Joe Conway credativ LLC: http://www.credativ.us Linux, PostgreSQL, and general Open Source Training, Service, Consulting, 24x7 Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- 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] Feature Request: pg_replication_master()
On Fri, Dec 21, 2012 at 9:21 AM, Simon Riggs si...@2ndquadrant.com wrote: No, lets not. The only stall happening is because of a refusal to listen to another person's reasonable request during patch review. That requirement is not a blocker to the idea, it just needs to be programmed. Lets just implement the reasonable request for backwards compatibility, rather than wasting time on reopening the debate. I read this as let's do it the way I proposed, instead of the way other people proposed. I don't see how that suggestion advances the debate. If I recall correctly, and I might not, because it's been a year, you wanted to implicitly include recovery.conf in postgresql.conf only when the system is recovery mode, but that gave rise to a bunch of thorny definitional issues that were never adequately solved. I would have been willing to tolerate that solution if they had been, but they were not. It is not accurate to suggest that you presented a reasonable proposal and other people refused to listen. That is not what happened. -- 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] Writing Trigger Functions in C
Date: Fri, 21 Dec 2012 11:56:25 -0500 Subject: Re: [HACKERS] Writing Trigger Functions in C From: cbbro...@gmail.com To: charle...@outlook.com CC: pgsql-hackers@postgresql.org On Fri, Dec 21, 2012 at 11:25 AM, Charles Gomes charle...@outlook.commailto:charle...@outlook.com wrote: Hello guys, I've been finding performance issues when using a trigger to modify inserts on a partitioned table. If using the trigger the total time goes from 1 Hour to 4 hours. The trigger is pretty simple: CREATE OR REPLACE FUNCTION quotes_insert_trigger() RETURNS trigger AS $ BEGIN EXECUTE 'INSERT INTO quotes_'|| to_char(new.received_time,'_MM_DD') ||' VALUES (($1).*)' USING NEW ; RETURN NULL; END; $ LANGUAGE plpgsql; I've seen that some of you guys have worked on writing triggers in C. Does anyone have had an experience writing a trigger for partitioning in C ? I'd want to be very careful about assuming that implementing the trigger function in C would necessarily improve performance. It's pretty likely that it wouldn't help much, as a fair bit of the cost of firing a trigger have to do with figuring out which function to call, marshalling arguments, and calling the function, none of which would magically disappear by virtue of implementing in C. A *major* cost that your existing implementation has is that it's re-planning the queries for every single invocation. This is an old, old problem from the Lisp days, EVAL considered evil http://stackoverflow.com/questions/2571401/why-exactly-is-eval-evil The EXECUTE winds up replanning queries every time the trigger fires. If you can instead enumerate the partitions explicitly, putting them into (say) a CASE clause, the planner could generate the plan once, rather than a million times, which would be a HUGE savings, vastly greater than you could expect from recoding into C. The function might look more like: create or replace function quotes_insert_trigger () returns trigger as $$ declare c_rt text; begin c_rt := to_char(new.received_time, '_MM_DD'); case c_rt when '2012_03_01' then insert into 2012_03_01 values (NEW.*) using new; when '2012_03_02' then insert into 2012_03_02 values (NEW.*) using new; else raise exception 'Need a new partition function for %', c_rt; end case; end $$ language plpgsql; You'd periodically need to change the function to reflect the existing set of partitions, but that's cheaper than creating a new partition. The case statement gets more expensive (in effect O(n) on the number of partitions, n) as the number of partitions increases. You could split the date into pieces (e.g. - years, months, days) to diminish that cost. But at any rate, this should be *way* faster than what you're running now, and not at any heinous change in development costs (as would likely be the case reimplementing using SPI). -- When confronted by a difficult problem, solve it by reducing it to the question, How would the Lone Ranger handle this? I will change and implement it this way, I was not aware of such optimization. Will post back after my benchmark runs. -- 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] Feature Request: pg_replication_master()
On 21 December 2012 17:12, Robert Haas robertmh...@gmail.com wrote: On Fri, Dec 21, 2012 at 9:21 AM, Simon Riggs si...@2ndquadrant.com wrote: No, lets not. The only stall happening is because of a refusal to listen to another person's reasonable request during patch review. That requirement is not a blocker to the idea, it just needs to be programmed. Lets just implement the reasonable request for backwards compatibility, rather than wasting time on reopening the debate. I read this as let's do it the way I proposed, instead of the way other people proposed. I don't see how that suggestion advances the debate. If I recall correctly, and I might not, because it's been a year, you wanted to implicitly include recovery.conf in postgresql.conf only when the system is recovery mode, but that gave rise to a bunch of thorny definitional issues that were never adequately solved. I would have been willing to tolerate that solution if they had been, but they were not. It is not accurate to suggest that you presented a reasonable proposal and other people refused to listen. That is not what happened. Having looked into how to solve it, I think its solvable easily enough. If the energy expended on that thread, and now this one was directed to actually solve the problem, it would be solved. Characterising the problem as containing a bunch of thorny definitional issues is just a way to further avoid that, unintentionally or not. Whether you think you can or think you can't, either way you are right - Henry Ford -- Simon Riggs 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] improving PL/Python builds on OS X
The PL/Python build on OS X is currently hardcoded to use the system
Python install. If you try to override this when running configure, you
get a mysterious mix-and-match build. If you want to build against your
own Python build, or MacPorts or Homebrew, you can't.
This is straightforward to fix. In configure, besides checking Python
include and library paths, we can also check whether it's a framework
build and the right parameters for that. The attached patch does that
and does the job for me. Please test it.
One constraint, which is explained in the comment in
src/pl/plpython/Makefile is that in Python 2.5, there is no official
way to detect either framework builds or shared libpython builds, so we
can't support those versions on OS X, at least without more hardcoding
of things. I'd rather phase some of that out, but if someone needs to
continue to use Python 2.4 or earlier on OS X, let me know. (Or more
proper fix would be to split DLSUFFIX into two variables, but that seems
more work than it's worth right now.)
diff --git a/config/python.m4 b/config/python.m4
index 663ccf9..af4d8d7 100644
--- a/config/python.m4
+++ b/config/python.m4
@@ -48,7 +48,6 @@ AC_MSG_RESULT([$python_includespec])
AC_SUBST(python_majorversion)[]dnl
AC_SUBST(python_version)[]dnl
-AC_SUBST(python_configdir)[]dnl
AC_SUBST(python_includespec)[]dnl
])# _PGAC_CHECK_PYTHON_DIRS
@@ -69,8 +68,14 @@ python_libdir=`${PYTHON} -c import distutils.sysconfig;
print(' '.join(filter(N
python_ldlibrary=`${PYTHON} -c import distutils.sysconfig; print('
'.join(filter(None,distutils.sysconfig.get_config_vars('LDLIBRARY'`
python_so=`${PYTHON} -c import distutils.sysconfig; print('
'.join(filter(None,distutils.sysconfig.get_config_vars('SO'`
ldlibrary=`echo ${python_ldlibrary} | sed s/${python_so}$//`
+python_framework=`${PYTHON} -c import distutils.sysconfig; print('
'.join(filter(None,distutils.sysconfig.get_config_vars('PYTHONFRAMEWORK'`
+python_enable_shared=`${PYTHON} -c import distutils.sysconfig;
print(distutils.sysconfig.get_config_vars().get('Py_ENABLE_SHARED',0))`
-if test x${python_libdir} != x -a x${python_ldlibrary} != x -a
x${python_ldlibrary} != x${ldlibrary}
+if test -n $python_framework; then
+ python_frameworkprefix=`${PYTHON} -c import distutils.sysconfig;
print('
'.join(filter(None,distutils.sysconfig.get_config_vars('PYTHONFRAMEWORKPREFIX'`
+ python_libspec=-F $python_frameworkprefix -framework $python_framework
+ python_enable_shared=1
+elif test x${python_libdir} != x -a x${python_ldlibrary} != x -a
x${python_ldlibrary} != x${ldlibrary}
then
# New way: use the official shared library
ldlibrary=`echo ${ldlibrary} | sed s/^lib//`
@@ -86,13 +91,16 @@ else
python_libspec=-L${python_libdir} -lpython${python_ldversion}
fi
-python_additional_libs=`${PYTHON} -c import distutils.sysconfig; print('
'.join(filter(None,distutils.sysconfig.get_config_vars('LIBS','LIBC','LIBM','BASEMODLIBS'`
+if test -z $python_framework; then
+ python_additional_libs=`${PYTHON} -c import distutils.sysconfig;
print('
'.join(filter(None,distutils.sysconfig.get_config_vars('LIBS','LIBC','LIBM','BASEMODLIBS'`
+fi
AC_MSG_RESULT([${python_libspec} ${python_additional_libs}])
AC_SUBST(python_libdir)[]dnl
AC_SUBST(python_libspec)[]dnl
AC_SUBST(python_additional_libs)[]dnl
+AC_SUBST(python_enable_shared)[]dnl
# threaded python is not supported on OpenBSD
AC_MSG_CHECKING(whether Python is compiled with thread support)
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 9cc14da..ecfb801 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -175,11 +175,11 @@ enable_dtrace = @enable_dtrace@
enable_coverage= @enable_coverage@
enable_thread_safety = @enable_thread_safety@
+python_enable_shared = @python_enable_shared@
python_includespec = @python_includespec@
python_libdir = @python_libdir@
python_libspec = @python_libspec@
python_additional_libs = @python_additional_libs@
-python_configdir = @python_configdir@
python_majorversion= @python_majorversion@
python_version = @python_version@
diff --git a/src/pl/plpython/Makefile b/src/pl/plpython/Makefile
index afd8dea..e9b5e3c 100644
--- a/src/pl/plpython/Makefile
+++ b/src/pl/plpython/Makefile
@@ -5,13 +5,20 @@ top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
-# On some platforms we can only build PL/Python if libpython is a
-# shared library. Since there is no official way to determine this
-# (at least not in pre-2.3 Python), we see if there is a file that is
-# named like a shared library.
+# We need libpython as a shared library. In Python =2.5, configure
+# asks Python directly. But because this has been broken in Debian
+# for a long time (http://bugs.debian.org/695979), and to support
+# older Python versions, we see if there is a file that is named like
+# a shared library
Re: [HACKERS] Feature Request: pg_replication_master()
On 21 December 2012 14:09, Robert Haas robertmh...@gmail.com wrote: On Thu, Dec 20, 2012 at 5:07 PM, Joshua Berkus j...@agliodbs.com wrote: As ever, we spent much energy on debating backwards compatibility rather than just solving the problem it posed, which is fairly easy to solve. Well, IIRC, the debate was primarily of *your* making. Almost everyone else on the thread was fine with the original patch, and it was nearly done for 9.2 before you stepped in. I can't find anyone else on that thread who thought that backwards compatibility was more important than fixing the API. Then you're forgetting the thread. If you reread it you will find I was not alone. +1. Let's JFDI. Agreed, lets get on with solving the problem rather than continuing a debate that could have been avoided, and can stop any time people choose to let it. -- Simon Riggs 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] Switching timeline over streaming replication
On 21.12.2012 01:50, Thom Brown wrote: Now I'm getting this on all standbys after promoting the first standby in a chain. ... TRAP: FailedAssertion(!(((sentPtr)= (SendRqstPtr))), File: walsender.c, Line: 1425) Sigh. I'm sounding like a broken record, but I just committed another fix for this, should work now. - 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] tuplesort memory usage: grow_memtuples
On 8 December 2012 14:41, Andres Freund and...@2ndquadrant.com wrote: Is anybody planning to work on this? There hasn't been any activity since the beginning of the CF and it doesn't look like there is much work left? I took another look at this. The growmemtuples bool from Jeff's original patch has been re-added. My strategy for preventing overflow is to use a uint64, and to use Min()/Max() as appropriate. As Robert mentioned, even a 64-bit integer could overflow here, and I account for that. Actually, right now this is only a theoretical problem on 64-bit platforms, because of the MaxAllocSize limitation - allowedMem being more than 2^38 (bytes, or 256GB) is a situation in which we won't repalloc anyway, because of this: /* * On a 64-bit machine, allowedMem could be high enough to get us into * trouble with MaxAllocSize, too. */ ! if ((Size) (newmemtupsize) = MaxAllocSize / sizeof(SortTuple)) ! goto noalloc; I reintroduced this check, absent in prior revisions, positioned around the new code: ! /* We assume here that the memory chunk overhead associated with the !* memtuples array is constant and so there will be no unexpected addition !* to what we ask for. (The minimum array size established in !* tuplesort_begin_common is large enough to force palloc to treat it as a !* separate chunk, so this assumption should be good. But let's check it, !* since the above fall-back may be used.) */ if (state-availMem = (long) (state-memtupsize * sizeof(SortTuple))) return false; Though we use a uint64 for memtupsize here, we still don't fully trust the final value: ! newmemtupsize = Min(Max(memtupsize * allowedMem / memNowUsed, ! memtupsize), ! memtupsize * 2); I also added a brief note within tuplestore.c to the effect that the two buffer sizing strategies are not in sync. Thoughts? -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services sortmem_grow-v5.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] Commits 8de72b and 5457a1 (COPY FREEZE)
On 11 December 2012 03:01, Noah Misch n...@leadboat.com wrote: On Mon, Dec 10, 2012 at 08:04:55PM -0500, Robert Haas wrote: I think the current behavior, where we treat FREEZE as a hint, is just awful. Regardless of whether the behavior is automatic or manually requested, the idea that you might get the optimization or not depending on the timing of relcache flushes seems very much undesirable. I mean, if the optimization is actually important for performance, then you want to get it when you ask for it. If it isn't, then why bother having it at all? Let's say that COPY FREEZE normally doubles performance on a data load that therefore takes 8 hours - somebody who suddenly loses that benefit because of a relcache flush that they can't prevent or control and ends up with a 16 hour data load is going to pop a gasket. Until these threads, I did not know that a relcache invalidation could trip up the WAL avoidance optimization, and now this. I poked at the relevant relcache.c code, and it already takes pains to preserve the needed facts. The header comment of RelationCacheInvalidate() indicates that entries bearing an rd_newRelfilenodeSubid can safely survive the invalidation, but the code does not implement that. I think the comment is right, and this is just an oversight in the code going back to its beginning (fba8113c). I think the comment is right also and so the patch is good. I will apply, barring objections. The information is only ever non-zero inside a single backend. There isn't any reason I can see why we wouldn't be able to remember this information in all cases, perhaps with a few push-ups. I doubt the comment at the declaration of rd_createSubid in rel.h, though I can't presently say what correct comment should replace it. rd_createSubid certainly does *not* get blown away by a message overflow as copy.c claims. I can't see any other way for a message overflow to cause it to be reset. I can no longer see a reason for us to regard the rd_createSubid as merely a hint. So we should change copy.c also. CLUSTER does preserve the old value, at least for the main table relation. CLUSTER probably should *set* rd_newRelfilenodeSubid. I can't see a reason not to do this in terms of correctness. However, I can't see any reason why you'd want to CLUSTER a table and then load more data into it in the same transaction, so I'm tempted to just leave that as is to avoid introducing other bugs. But I dare say people will think we should fix it there also. -- Simon Riggs 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] enhanced error fields
On 11 December 2012 13:01, Pavel Stehule pavel.steh...@gmail.com wrote: There are two basic aspects of design * usability - we would to remove necessity of parsing error messages for getting interesting data, it decrease dependency on current error messages - then I am thinking so some informations about triggers or functions where exception was born are practical (current implementation is different task - we can find better solution than I proposed highly probably) * compatibility - personally, lot of diagnostics fields are very vague defined, so here can be lot of issues - I have no experience with DB2 or Oracle, so I cannot to speak more, but we should to say, what we expect. Me neither. I afraid so constraint name is not afraid I'm sorry, I don't follow you here. postgres=# create table a (a int primary key); CREATE TABLE postgres=# create table b (b int references a(a)); CREATE TABLE postgres=# insert into b values (10); ERROR: insert or update on table b violates foreign key constraint b_b_fkey DETAIL: Key (b)=(10) is not present in table a. postgres=# \set VERBOSITY verbose postgres=# insert into b values (10); ERROR: 23503: insert or update on table b violates foreign key constraint b_b_fkey DETAIL: Key (b)=(10) is not present in table a. LOCATION: ri_ReportViolation, ri_triggers.c:3222 postgres=# insert into a values(10); INSERT 0 1 postgres=# insert into b values (10); INSERT 0 1 postgres=# delete from a; ERROR: 23503: update or delete on table a violates foreign key constraint b_b_fkey on table b DETAIL: Key (a)=(10) is still referenced from table b. LOCATION: ri_ReportViolation, ri_triggers.c:3232 there are two different bugs - with same ERRCODE and constraint name - once is problem on b, second on a so constraint_table is interesting information Really? I'm not so sure that that's a distinct that the user would have to care about, or at least care much about. I defer to others here. Certainly, I am generally conscious of the fact that we don't want to create an excessive number of fields. My implementation is probably too dumb - but a question - where exception is coming from? is relative typical - but we can to clean implementation. I see a issue in definition. Usually I am interesting about most deep custom code - not most deep code. I think that knowing where an exception comes from is not useful information for end-users. Therefore, I am unconvinced of the need to present information to end users that is based on that. That's my difficulty. so I am not against to removing some fields, but constraint_table and routine_name is useful and we should to produce this information. I'm afraid that you and I differ on that. To make logging less verbose, TABLE NAME isn't consistently split out as a separate field - this seems fine to me, since application code doesn't target logs: uff, no - I don't like it - it is not consistent and I don't think so one row more is a issue. But is a question if we don't need a second VERBOSE level for showing these fields - maybe VERBOSE_ENHANCED or some similar VERBOSE_ENHANCED? I'm not sure about that. If you think it's important for them to be consistent, I concede the point. we don't need log these fields usually ?? Well, I don't think we *usually* need to log *any* verbose fields. -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and 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] [WIP] pg_ping utility
On Wed, Dec 19, 2012 at 8:28 AM, Michael Paquier michael.paqu...@gmail.com wrote: On Wed, Dec 12, 2012 at 12:06 AM, Bruce Momjian br...@momjian.us wrote: On Sat, Dec 8, 2012 at 08:59:00AM -0500, Phil Sorber wrote: On Sat, Dec 8, 2012 at 7:50 AM, Michael Paquier michael.paqu...@gmail.com wrote: Bruce mentionned that pg_isready could be used directly by pg_ctl -w. Default as being non-verbose would make sense. What are the other tools you are thinking about? Some utilities in core? I think Bruce meant that PQPing() is used by pg_ctl -w, not that he would use pg_isready. Right. OK cool. If you have some spare room to write a new version with verbose option as default, I'll be pleased to review it and then write it as ready for committer. Added new version with default verbose and quiet option. Also updated docs to reflect changes. -- Michael Paquier http://michael.otacoo.com pg_isready_bin_v8.diff Description: Binary data pg_isready_docs_v8.diff 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] PL/PgSQL STRICT
On Fri, Dec 21, 2012 at 4:53 PM, Tom Lane t...@sss.pgh.pa.us wrote: That was my first reaction too, but Marko's followon examples seem to make a reasonable case for it. There are many situations where you expect an UPDATE or DELETE to hit exactly one row. Often, programmers won't bother to add code to check that it did ... but if a one-word addition to the command can provide such a check, it seems more likely that they would add the check. Very true. When I was a PL/PgSQL beginner a few years ago I did exactly that, I didn't check if the update actually updated any row, I didn't know it could fail, and felt extremely worried and stupid when I realised this. I spent an entire day going through all functions fixing this problem at all places. The fix was not beautiful and it bugged me there was not a prettier way to fix it. -- 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] Feature Request: pg_replication_master()
On Fri, Dec 21, 2012 at 02:25:47PM +, Simon Riggs wrote: On 20 December 2012 19:29, Bruce Momjian br...@momjian.us wrote: At this point backward compatibility has paralyzed us from fixing a recovery.conf API that everyone agrees is non-optimal, and this has gone on for multiple major releases. I don't care what we have to do, just clean this up for 9.3! The main stall at this point is that the developer who wrote that patch no longer spends much time working on Postgres. AFAICS there is nobody working on this for 9.3 mainly because its not a priority, nor will implementing that fix the OP's request. The job wasn't completed because the demands for backward compatibility were too complex from a coding/user experience perspective, so the original developer just went away. There is no paralysis because there never was a blocker, only a request for backwards compatibility, which is easily possible to implement. OK, and I am saying the request for backwards compatibility is rejected. You want to have a vote on that right now? And don't make your typical demands that you will not 'accept' something that isn't backward compatible. I don't care if you accept anything or not, we are moving ahead, with or without you. If we can't get beyond this, I need to start blogging at how insular our developer team is and how we need new people to joint the hackers list and we can start this discussion all over again with a new group. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] Switching timeline over streaming replication
On 21 December 2012 18:13, Heikki Linnakangas hlinnakan...@vmware.comwrote: On 21.12.2012 01:50, Thom Brown wrote: Now I'm getting this on all standbys after promoting the first standby in a chain. ... TRAP: FailedAssertion(!(((sentPtr)**= (SendRqstPtr))), File: walsender.c, Line: 1425) Sigh. I'm sounding like a broken record, but I just committed another fix for this, should work now. Thanks Heikki. Just quickly retested with a new set of 120 standbys and all looks fine as far as the logs are concerned: LOG: replication terminated by primary server DETAIL: End of WAL reached on timeline 1 LOG: record with zero length at 0/37902A0 LOG: fetching timeline history file for timeline 2 from primary server LOG: restarted WAL streaming at 0/300 on timeline 1 LOG: replication terminated by primary server DETAIL: End of WAL reached on timeline 1 LOG: new target timeline is 2 LOG: restarted WAL streaming at 0/300 on timeline 2 LOG: replication terminated by primary server DETAIL: End of WAL reached on timeline 2 LOG: record with zero length at 0/643E248 LOG: fetching timeline history file for timeline 3 from primary server LOG: restarted WAL streaming at 0/600 on timeline 2 LOG: replication terminated by primary server DETAIL: End of WAL reached on timeline 2 LOG: new target timeline is 3 LOG: restarted WAL streaming at 0/600 on timeline 3 LOG: replication terminated by primary server DETAIL: End of WAL reached on timeline 3 LOG: record with zero length at 0/6BB13A8 LOG: fetching timeline history file for timeline 4 from primary server LOG: restarted WAL streaming at 0/600 on timeline 3 LOG: replication terminated by primary server DETAIL: End of WAL reached on timeline 3 LOG: new target timeline is 4 LOG: restarted WAL streaming at 0/600 on timeline 4 -- Thom
Re: [HACKERS] Feature Request: pg_replication_master()
On 21 December 2012 19:21, Bruce Momjian br...@momjian.us wrote: On Fri, Dec 21, 2012 at 02:25:47PM +, Simon Riggs wrote: On 20 December 2012 19:29, Bruce Momjian br...@momjian.us wrote: At this point backward compatibility has paralyzed us from fixing a recovery.conf API that everyone agrees is non-optimal, and this has gone on for multiple major releases. I don't care what we have to do, just clean this up for 9.3! The main stall at this point is that the developer who wrote that patch no longer spends much time working on Postgres. AFAICS there is nobody working on this for 9.3 mainly because its not a priority, nor will implementing that fix the OP's request. The job wasn't completed because the demands for backward compatibility were too complex from a coding/user experience perspective, so the original developer just went away. It's not too complex. You just want that to be true. The original developer has actually literally gone away, but not because of this. There is no paralysis because there never was a blocker, only a request for backwards compatibility, which is easily possible to implement. OK, and I am saying the request for backwards compatibility is rejected. You want to have a vote on that right now? And don't make your typical demands that you will not 'accept' something that isn't backward compatible. I don't care if you accept anything or not, we are moving ahead, with or without you. If we can't get beyond this, I need to start blogging at how insular our developer team is and how we need new people to joint the hackers list and we can start this discussion all over again with a new group. Yes, I think having some people on this list who make decisions after they have heard technical facts would be a welcome change. -- Simon Riggs 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] Feature Request: pg_replication_master()
On Fri, Dec 21, 2012 at 07:32:48PM +, Simon Riggs wrote: On 21 December 2012 19:21, Bruce Momjian br...@momjian.us wrote: On Fri, Dec 21, 2012 at 02:25:47PM +, Simon Riggs wrote: On 20 December 2012 19:29, Bruce Momjian br...@momjian.us wrote: At this point backward compatibility has paralyzed us from fixing a recovery.conf API that everyone agrees is non-optimal, and this has gone on for multiple major releases. I don't care what we have to do, just clean this up for 9.3! The main stall at this point is that the developer who wrote that patch no longer spends much time working on Postgres. AFAICS there is nobody working on this for 9.3 mainly because its not a priority, nor will implementing that fix the OP's request. The job wasn't completed because the demands for backward compatibility were too complex from a coding/user experience perspective, so the original developer just went away. It's not too complex. You just want that to be true. The original developer has actually literally gone away, but not because of this. Well, Robert and I remember it differently. Anyway, I will ask for a vote now. There is no paralysis because there never was a blocker, only a request for backwards compatibility, which is easily possible to implement. OK, and I am saying the request for backwards compatibility is rejected. You want to have a vote on that right now? And don't make your typical demands that you will not 'accept' something that isn't backward compatible. I don't care if you accept anything or not, we are moving ahead, with or without you. If we can't get beyond this, I need to start blogging at how insular our developer team is and how we need new people to joint the hackers list and we can start this discussion all over again with a new group. Yes, I think having some people on this list who make decisions after they have heard technical facts would be a welcome change. OK, I will start blogging too. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] Feature Request: pg_replication_master()
On 21 December 2012 19:35, Bruce Momjian br...@momjian.us wrote: It's not too complex. You just want that to be true. The original developer has actually literally gone away, but not because of this. Well, Robert and I remember it differently. Anyway, I will ask for a vote now. And what will you ask for a vote on? Why not spend that effort on solving the problem? Why is it OK to waste so much time? Having already explained how to do this, I'll add backwards compatibility within 1 day of the commit of the patch you claim was blocked by this. I think it will take me about an hour and not be very invasive, just to prove what a load of hot air is being produced here. Yes, I think having some people on this list who make decisions after they have heard technical facts would be a welcome change. OK, I will start blogging too. Good for you. I'll stick to trying to improve PostgreSQL by coding. -- Simon Riggs 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] Request for vote to move forward with recovery.conf overhaul
There has been discussion in the past of removing or significantly changing the way streaming replication/point-in-time-recovery (PITR) is setup in Postgres. Currently the file recovery.conf is used, but that was designed for PITR and does not serve streaming replication well. This all should have been overhauled when streaming replication was added in 2010 in Postgres 9.0. However, time constraints and concern about backward compatibility has hampered this overhaul. At this point, backward compatibility seems to be hampering our ability to move forward. I would like a vote that supports creation of a new method for setting up streaming replication/point-in-time-recovery, where backward compatibility is considered only where it is minimally invasive. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] Feature Request: pg_replication_master()
On Fri, Dec 21, 2012 at 07:43:29PM +, Simon Riggs wrote: On 21 December 2012 19:35, Bruce Momjian br...@momjian.us wrote: It's not too complex. You just want that to be true. The original developer has actually literally gone away, but not because of this. Well, Robert and I remember it differently. Anyway, I will ask for a vote now. And what will you ask for a vote on? Why not spend that effort on solving the problem? Why is it OK to waste so much time? I have already sent out the vote request. Having already explained how to do this, I'll add backwards compatibility within 1 day of the commit of the patch you claim was blocked by this. I think it will take me about an hour and not be very invasive, just to prove what a load of hot air is being produced here. I have seen too many cases where things are derailed. I think we need a clear statement of exactly how important backward compatibility is in this case. I think the fear of you requesting stuff has basically scared everyone away from working on this. Of course, I might be wrong, but I have to make a guess on this one. Yes, I think having some people on this list who make decisions after they have heard technical facts would be a welcome change. OK, I will start blogging too. Good for you. I'll stick to trying to improve PostgreSQL by coding. Well, when our process is broken, coding doesn't really solve the problem, at least from my perspective. Let me explain it this way. A family has a car they love, but the car is in bad shape, so they all go to the auto dealership. They like some features of the new cars, but it doesn't have everying the old car had, so they go to another dealership, and then another. Two years go buy, and they still haven't gotten a new car, and the old car is getting worse. As some point, someone in the family has to stand up and say, Hey we aren't going to get everything we liked in the old car, but we have to make a decision and move forward. That is where we are now. Overhauling recovery.conf can't be a super-complex job, and we already have a patch we can base it of off. Why is this not done yet! I don't know, but I have seen lots of discussion about it, and no clear conclusions, at least that you agree with. I have realized this could languish for another two years unless I stand up, say the old car is dead, and force us to get a new car! -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] Commits 8de72b and 5457a1 (COPY FREEZE)
On Fri, Dec 21, 2012 at 06:47:56PM +, Simon Riggs wrote: On 11 December 2012 03:01, Noah Misch n...@leadboat.com wrote: Until these threads, I did not know that a relcache invalidation could trip up the WAL avoidance optimization, and now this. I poked at the relevant relcache.c code, and it already takes pains to preserve the needed facts. The header comment of RelationCacheInvalidate() indicates that entries bearing an rd_newRelfilenodeSubid can safely survive the invalidation, but the code does not implement that. I think the comment is right, and this is just an oversight in the code going back to its beginning (fba8113c). I think the comment is right also and so the patch is good. I will apply, barring objections. The information is only ever non-zero inside a single backend. There isn't any reason I can see why we wouldn't be able to remember this information in all cases, perhaps with a few push-ups. I doubt the comment at the declaration of rd_createSubid in rel.h, though I can't presently say what correct comment should replace it. rd_createSubid certainly does *not* get blown away by a message overflow as copy.c claims. I can't see any other way for a message overflow to cause it to be reset. I can no longer see a reason for us to regard the rd_createSubid as merely a hint. So we should change copy.c also. I thought of one case where we do currently forget rd_newRelfilenodeSubid: BEGIN; TRUNCATE t; SAVEPOINT save; TRUNCATE t; ROLLBACK TO save; I don't mind that one too much. CLUSTER does preserve the old value, at least for the main table relation. CLUSTER probably should *set* rd_newRelfilenodeSubid. I can't see a reason not to do this in terms of correctness. However, I can't see any reason why you'd want to CLUSTER a table and then load more data into it in the same transaction, so I'm tempted to just leave that as is to avoid introducing other bugs. But I dare say people will think we should fix it there also. I could see using that capability occasionally, but I wouldn't mix such a change in with the goals of this thread. Thanks, nm -- 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] Feature Request: pg_replication_master()
On 21.12.2012 21:43, Simon Riggs wrote: On 21 December 2012 19:35, Bruce Momjianbr...@momjian.us wrote: It's not too complex. You just want that to be true. The original developer has actually literally gone away, but not because of this. Well, Robert and I remember it differently. Anyway, I will ask for a vote now. And what will you ask for a vote on? Why not spend that effort on solving the problem? Why is it OK to waste so much time? Having already explained how to do this, I'll add backwards compatibility within 1 day of the commit of the patch you claim was blocked by this. I think it will take me about an hour and not be very invasive, just to prove what a load of hot air is being produced here. I haven't been following this.. Could you two post a link to the patch we're talking about, and the explanation of how to add backwards compatibility to it? Just by looking at the last few posts, this seems like a no brainer. The impression I get is that there's a patch that's otherwise ready to be applied, but Simon and some others want to have backwards-compatiblity added to it first. And Simon has a plan on how to do it, and can do it in one day. The obvious solution is that Simon posts the patch, with the backwards-compatibility added. We can then discuss that, and assuming no surprises there, commit it. And everyone lives happily ever after. - 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] Feature Request: pg_replication_master()
On 12/21/2012 11:56 AM, Bruce Momjian wrote: That is where we are now. Overhauling recovery.conf can't be a super-complex job, and we already have a patch we can base it of off. Why is this not done yet! I don't know, but I have seen lots of discussion about it, and no clear conclusions, at least that you agree with. I have realized this could languish for another two years unless I stand up, say the old car is dead, and force us to get a new car! Forgive me because I have been up for 28 hours on a 9.0 to 9.2 migration with Hot Standby and PgPool-II for load balancing but I was excessively irritated that I had to go into recovery.conf to configure things. I am one of the software authors that breaking recovery.conf will cause problems for. Break it anyway. The fact that anyone has to touch recovery.conf is at this point in software development, backwards. No matter how it gets implemented it shouldn't be anymore complicated than: Master: SET/ENABLE replication FOR db1; Standby: SET/ENABLE hot_standby MASTER db0 Have the logs automatically go to an archive tablespace (so it can be moved) Now I am not expecting this to happen or even saying it is a valid approach. What I am saying is that it should be THAT easy [1]. [2] Multi version rpms on the same box still need some help (lib issues) [3] Bruce, you rock... pg_ugrade is the bomb Joshua D. Drake 1. Obviously I am talking about the simplest of configuration and there are a whole slew of other options that needs to be set but have reasonable defaults. -- Command Prompt, Inc. - http://www.commandprompt.com/ PostgreSQL Support, Training, Professional Services and Development High Availability, Oracle Conversion, Postgres-XC @cmdpromptinc - 509-416-6579 -- 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] pgcrypto seeding problem when ssl=on
This should have gone to secur...@postgresql.org, instead. On Fri, Dec 21, 2012 at 06:05:10PM +0200, Marko Kreen wrote: When there is 'ssl=on' then postmaster calls SSL_CTX_new(), which asks for random number, thus requiring initialization of randomness pool (RAND_poll). After that all forked backends think pool is already initialized. Thus they proceed with same fixed state they got from postmaster. Attached patch makes both gen_random_bytes() and pgp_encrypt() seed pool with output from gettimeofday(), thus getting pool off from fixed state. Basically, this mirrors what SSL_accept() already does. That adds only 10-20 bits of entropy. Is that enough? How about instead calling RAND_cleanup() after each backend fork? Thanks, nm -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch für MAP_HUGETLB for mmap() shared memory
On 2012-10-30 21:16:07 +0100, Christian Kruse wrote:
+static long
+InternalGetHugepageSize()
+{
+ DIR *dir = opendir(HUGE_PAGE_INFO_DIR);
+ long smallest_size = -1, size;
+ char *ptr;
...
+ while((ent = readdir(dir)) != NULL)
+ {
This should be (Allocate|Read)Dir btw.
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] Event Triggers: adding information
Hi, On 2012-12-21 09:48:22 -0500, Robert Haas wrote: On Wed, Dec 12, 2012 at 4:47 PM, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: The previously attached patch already needs a rebase since Tom fixed the single user mode where you're not supposed to access potentially corrupted system indexes. Please find attached a new version of the patch that should applies cleanly to master (I came to trust git). Sorry for the delay - I'm looking at this now. My first observation (a.k.a. gripe) is this patch touches an awful lot of code all over the backend. We just did a big old refactoring to try to get all the rename and alter owner commands to go through the same code path, but if this patch is any indication it has not done us a whole lot of good. The whole idea of all that work is that when people wanted to make systematic changes to those operations (like involving sepgsql, or returning the OID) there would not be 47 places to change. Apparently, we aren't there yet. Maybe we need some more refactoring of that stuff before tackling this? Its hard to do such refactorings without very concrete use-cases, so I think its not unlikely that this patch points out some things that can be committed independently. ISTM that a good part of the return oid; changes are actually such a part. Does anyone object to the idea of making every command that creates a new SQL object return the OID of an object, and similarly for RENAME / ALTER TO? If so, maybe we ought to go through and do those things first, as separate patches, and then rebase this over those changes for simplicity. I can probably do that real soon now, based on this patch, if there are no objections, but I don't want to do the work and then have someone say, ack, what have you done? FWIW I am all for it. I'm basically implacably opposed to the ddl_rewrite.c part of this patch. I think that the burden of maintaining reverse-parsing code for all the DDL we support is an unacceptable level of maintenance overhead for this feature. It essentially means that every future change to gram.y that affects any of the supported commands will require a compensating change to ddl_rewrite.c. That is a whole lot of work and it is almost guaranteed that future patch authors will sometimes fail to do it correctly. Thats - unsurprisingly - where I have a different opinion. ruleutils.c does that for normal SQL and while DDL is a bit bigger in the grammar than the DQL support I would say the data structures of of the latter are far more complex than for DDL. If you look at what changes were made to ruleutils.c in the last years the rate of bugs isn't, as far as I can see, higher than in other areas of the code. The ruleutils.c precedent also means that patch authors *have* to be aware of it already. Not too many features get introduced that only change DDL. At an absolute bare minimum I think we would need some kind of comprehensive test suite for this functionality, as opposed to the less than 60 lines of new test cases this patch adds which completely fail to test this code in any way at all, but really I think we should just not have it. Absolutely aggreed. It WILL get broken (if it isn't already) and it WILL slow down future development of SQL features. It also WILL have edge cases where it doesn't work properly, such as where the decision of the actual index name to use is only decided at execution time and cannot be inferred from the parse tree. That obviousl needs to be solved, but I think the idea of invoking this command trigger whenever its appropriate (which I think was actually your idea?) should take care of most of the problems around this. I know that you feel that all of these are tolerable costs, but I 100% disgaree. I think that missing DDL support is one of the major weaknesses of all potential logical replication solutions. Be it slonly, londiste, BDR, or even something what some day may be in core. I just don't see any more realistic way to fix that besides supporting something like this. I predict that if we commit this, it will becomes a never-ending and irrecoverable font of trouble. Why? Its really not that different from whats done currently? 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] Review of Row Level Security
2012/12/21 Kevin Grittner kgri...@mail.com:
Simon Riggs wrote:
Each table has a single security clause. The clause doesn't enforce
that it must contain something that depends on role, but that is the
most easily understood usage of it. We do that to ensure that you can
embed the intelligence into a function, say hasRowLevelAccess(), which
doesn't have any provable relationship on role, and maybe wouldn't do
anything in unit test, yet clearly in production it would do so.
It would be easy enough to include read/write variable clauses by
using a function similar to TG_OP for triggers, e.g. StatementType.
That would make the security clause that applied only to reads into
something like this (StatementType('INSERT, UPDATE') OR other-quals).
In the case where the logic in encapsulated into a function, what
are the logical differences from a BEFORE EACH ROW trigger? If
none, and this is strictly an optimization, what are the benchmarks
showing?
It seems to me we need some more discussion about design and
implementation on row-security checks of writer-side, to reach our
consensus.
On the other hand, we are standing next to the consensus about
reader-side; a unique row-security policy (so, first version does not
support per-command policy) shall be checked on table scanning
on select, update or delete commands.
I'd like to suggest these arguable stuff being postponed to v9.4,
and we like to focus on the minimum / core functionality towards
the deadline of v9.3.
I think it is the most productive way to enhance our features.
I can understand Simon's opinion; security feature should not be
defective item. However, please don't forget sepgsql started from
just a small functional module to check DML permissions only.
How about folk's opinion?
Thanks,
--
KaiGai Kohei kai...@kaigai.gr.jp
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review of Row Level Security
KaiGai, all,
* Kohei KaiGai (kai...@kaigai.gr.jp) wrote:
2012/12/21 Kevin Grittner kgri...@mail.com:
Simon Riggs wrote:
Each table has a single security clause. The clause doesn't enforce
that it must contain something that depends on role, but that is the
most easily understood usage of it. We do that to ensure that you can
embed the intelligence into a function, say hasRowLevelAccess(), which
doesn't have any provable relationship on role, and maybe wouldn't do
anything in unit test, yet clearly in production it would do so.
It would be easy enough to include read/write variable clauses by
using a function similar to TG_OP for triggers, e.g. StatementType.
That would make the security clause that applied only to reads into
something like this (StatementType('INSERT, UPDATE') OR other-quals).
In the case where the logic in encapsulated into a function, what
are the logical differences from a BEFORE EACH ROW trigger? If
none, and this is strictly an optimization, what are the benchmarks
showing?
I'm trying to understand this piece also. It sounds like all we're
doing at the moment is adding different syntax to define a trigger
function and that's hardly what I, at least, was expecting. If a
function has to be written to have RLS, then I think we've failed. Much
of the point of this is to provide an easy solution which gets all the
details right for users who aren't comfortable or savvy enough to just
write functions/security definer views/etc themselves.
It seems to me we need some more discussion about design and
implementation on row-security checks of writer-side, to reach our
consensus.
Again, I agree with Kevin on this- there should be a wiki or similar
which actually outlines the high-level design, syntax, etc. That would
help us understand and be able to meaningfully comment about the
approach.
On the other hand, we are standing next to the consensus about
reader-side; a unique row-security policy (so, first version does not
support per-command policy) shall be checked on table scanning
on select, update or delete commands.
I don't feel that we've really reached a consensus about the
'reader-side' implemented in this patch- rather, we've agreed (at a
pretty high level) what the default impact of RLS for SELECT queries is.
While I'm glad that we were able to do that, I'm rather dismayed that it
took a great deal of discussion to get to that point.
Thanks,
Stephen
signature.asc
Description: Digital signature
Re: [HACKERS] pgcrypto seeding problem when ssl=on
On Fri, Dec 21, 2012 at 10:27 PM, Noah Misch n...@leadboat.com wrote: This should have gone to secur...@postgresql.org, instead. It went and this could have been patched in 9.2.2 but they did not care. On Fri, Dec 21, 2012 at 06:05:10PM +0200, Marko Kreen wrote: When there is 'ssl=on' then postmaster calls SSL_CTX_new(), which asks for random number, thus requiring initialization of randomness pool (RAND_poll). After that all forked backends think pool is already initialized. Thus they proceed with same fixed state they got from postmaster. Attached patch makes both gen_random_bytes() and pgp_encrypt() seed pool with output from gettimeofday(), thus getting pool off from fixed state. Basically, this mirrors what SSL_accept() already does. That adds only 10-20 bits of entropy. Is that enough? It's enough to get numbers that are not the same. Whether it's possible to guess next number when you know that there is only time() and getpid() added to fixed state (eg. those cleartext bytes in SSL handshake) - i don't know. In any case, this is how Postgres already operates SSL connections. How about instead calling RAND_cleanup() after each backend fork? Not instead - the gettimeofday() makes sense in any case. Considering that it's immediate problem only for pgcrypto, this patch has higher chance for appearing in back-branches. If the _cleanup() makes next RAND_bytes() call RAND_poll() again? Then yes, it certainly makes sense as core patch. -- marko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Making view dump/restore safe at the column-alias level
In commit 11e131854f8231a21613f834c40fe9d046926387 we rearranged ruleutils.c's handling of relation aliases to ensure that views can always be dumped and reloaded even in the face of confusing table renamings. I was reminded by http://archives.postgresql.org/pgsql-general/2012-12/msg00654.php that this is only half of the problem: you can still get burnt by ambiguous column references, and pretty easily at that. Aside from plain old ambiguity, there is a nastier problem: JOIN USING and NATURAL JOIN depend on particular column names matching up, which they might not do anymore after a column rename. We have discussed this previously (though I can't find the archives reference right now), and the best anybody came up with was to invent some syntax extension that would allow matching differently-named columns in USING, perhaps along the lines of USING (leftcol = rightcol, ...). But that's pretty ugly and nobody volunteered to actually do it. I had an idea though about how we might fix this without that. Assume that the problem is strictly ruleutils' to fix, ie we are not going to invent new syntax and we are not going to change the existing methods of assigning aliases to subselect columns. We clearly will need to let ruleutils assign new column aliases that are unique within each RTE entry. I think though that we can fix the JOIN USING problem if we introduce an additional idea that alias choices can be forced top-down. So a JOIN USING RTE would force the two columns being merged to be given the same alias already assigned to the merged column in the JOIN RTE. (If we ever get around to implementing the CORRESPONDING clause in UNION/INTERSECT/EXCEPT, it would have to do something similar.) We'd similarly force the output aliases at the top level of a view to be the view's known result column names (which presumably are distinct thanks to pg_attribute's unique constraint). Otherwise, as we descend the query tree, we can assign distinct column aliases to each column of an RTE, preferring the original name when possible but otherwise making it unique by adding a number, as we already did with the relation aliases. In the case of view-printing, once these aliases are all assigned we can represent them in the SQL output easily enough; that code is already there. I'm not sure whether it's a good idea for EXPLAIN to use this same kind of logic, since there's not currently anyplace in EXPLAIN output to show nondefault column aliases. It might be more confusing than otherwise to use generated aliases in EXPLAIN, even if the original aliases conflict. If we're going to do something like this, now (9.3) would be a good time since we already made changes in alias-assignment in the earlier commit. Comments, better ideas? 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] Commits 8de72b and 5457a1 (COPY FREEZE)
On 21 December 2012 20:10, Noah Misch n...@leadboat.com wrote: I thought of one case where we do currently forget rd_newRelfilenodeSubid: BEGIN; TRUNCATE t; SAVEPOINT save; TRUNCATE t; ROLLBACK TO save; That's a weird one. Aborting a subtransacton that sets it, when it was already set. The loss of rd_newRelfilenodeSubid in that case is deterministic, but tracking the full complexity of multiple relations and multiple nested subxids isn't worth the trouble for such rare cases [assumption]. I'd go for just setting an its_too_complex flag (with better name) that we can use to trigger a message in COPY to say that FREEZE option won't be honoured. That would then be completely consistent, rather than the lack of deterministic behaviour that Robert rightly objects to. -- Simon Riggs 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] Request for vote to move forward with recovery.conf overhaul
On 21 December 2012 19:46, Bruce Momjian br...@momjian.us wrote: There has been discussion in the past of removing or significantly changing the way streaming replication/point-in-time-recovery (PITR) is setup in Postgres. Currently the file recovery.conf is used, but that was designed for PITR and does not serve streaming replication well. This all should have been overhauled when streaming replication was added in 2010 in Postgres 9.0. However, time constraints and concern about backward compatibility has hampered this overhaul. At this point, backward compatibility seems to be hampering our ability to move forward. I would like a vote that supports creation of a new method for setting up streaming replication/point-in-time-recovery, where backward compatibility is considered only where it is minimally invasive. Given that I've said all along that I want change, I'll vote for that, especially since it is so reasonably worded. I also want backwards compatibility, so I would like whoever does this change to also spend some time on that, since it seems that the balance of time/cost is good enough to make it sensible to do so. I hope we can spend the time on that investigation, rather than further debate around what we mean by the word minimally. -- Simon Riggs 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] Review of Row Level Security
On 21 December 2012 22:01, Stephen Frost sfr...@snowman.net wrote: On the other hand, we are standing next to the consensus about reader-side; a unique row-security policy (so, first version does not support per-command policy) shall be checked on table scanning on select, update or delete commands. I don't feel that we've really reached a consensus about the 'reader-side' implemented in this patch- rather, we've agreed (at a pretty high level) what the default impact of RLS for SELECT queries is. While I'm glad that we were able to do that, I'm rather dismayed that it took a great deal of discussion to get to that point. Would anybody like to discuss this on a conference call on say 28th Dec, to see if we can agree a way forwards? I feel certain that we can work through any difficulties and agree a minimal subset for change. All comers welcome, just contact me offlist for details. I've got literally nothing riding on this, other than my desire for progress, so I don't see the need for going too many extra miles if nobody else does. -- Simon Riggs 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] Review of Row Level Security
Simon, * Simon Riggs (si...@2ndquadrant.com) wrote: Would anybody like to discuss this on a conference call on say 28th Dec, to see if we can agree a way forwards? I feel certain that we can work through any difficulties and agree a minimal subset for change. All comers welcome, just contact me offlist for details. Thank you for the offer, I agree that it's a good idea. I'd be happy to (and would like to) participate. I've got literally nothing riding on this, other than my desire for progress, so I don't see the need for going too many extra miles if nobody else does. I'd like to make progress also but let's make sure we're going in the right direction. :) Thanks again, Stephen signature.asc Description: Digital signature
[HACKERS] GRANT/REVOKE take NO lock on the target object?!
S1: rhaas=# create table foo (a int); CREATE TABLE rhaas=# begin; BEGIN rhaas=# lock foo; LOCK TABLE S2: rhaas=# grant all on foo to public; GRANT rhaas=# revoke all on foo from public; REVOKE This seems quite obviously silly, given the amount of time and energy we've spent worrying about ALTER TABLE lock levels. Note that GRANT/REVOKE on a table do a not-in-place update of the pg_class row; with anything less than an AccessExclusiveLock, the usual SnapshotNow hazards exist: another session can fail to find the pg_class row altogether. [ Credit: Noah Misch helped me trace down the problem that led me to this report. ] -- 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] Making view dump/restore safe at the column-alias level
On Fri, Dec 21, 2012 at 6:42 PM, Tom Lane t...@sss.pgh.pa.us wrote: In commit 11e131854f8231a21613f834c40fe9d046926387 we rearranged ruleutils.c's handling of relation aliases to ensure that views can always be dumped and reloaded even in the face of confusing table renamings. I was reminded by http://archives.postgresql.org/pgsql-general/2012-12/msg00654.php that this is only half of the problem: you can still get burnt by ambiguous column references, and pretty easily at that. Aside from plain old ambiguity, there is a nastier problem: JOIN USING and NATURAL JOIN depend on particular column names matching up, which they might not do anymore after a column rename. We have discussed this previously (though I can't find the archives reference right now), and the best anybody came up with was to invent some syntax extension that would allow matching differently-named columns in USING, perhaps along the lines of USING (leftcol = rightcol, ...). But that's pretty ugly and nobody volunteered to actually do it. I had an idea though about how we might fix this without that. Assume that the problem is strictly ruleutils' to fix, ie we are not going to invent new syntax and we are not going to change the existing methods of assigning aliases to subselect columns. We clearly will need to let ruleutils assign new column aliases that are unique within each RTE entry. I think though that we can fix the JOIN USING problem if we introduce an additional idea that alias choices can be forced top-down. So a JOIN USING RTE would force the two columns being merged to be given the same alias already assigned to the merged column in the JOIN RTE. (If we ever get around to implementing the CORRESPONDING clause in UNION/INTERSECT/EXCEPT, it would have to do something similar.) We'd similarly force the output aliases at the top level of a view to be the view's known result column names (which presumably are distinct thanks to pg_attribute's unique constraint). Otherwise, as we descend the query tree, we can assign distinct column aliases to each column of an RTE, preferring the original name when possible but otherwise making it unique by adding a number, as we already did with the relation aliases. In the case of view-printing, once these aliases are all assigned we can represent them in the SQL output easily enough; that code is already there. I'm not sure whether it's a good idea for EXPLAIN to use this same kind of logic, since there's not currently anyplace in EXPLAIN output to show nondefault column aliases. It might be more confusing than otherwise to use generated aliases in EXPLAIN, even if the original aliases conflict. If we're going to do something like this, now (9.3) would be a good time since we already made changes in alias-assignment in the earlier commit. Comments, better ideas? regards, tom lane I'm having a hard time following this. Can you provide a concrete example? -- 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] pgcrypto seeding problem when ssl=on
On Sat, Dec 22, 2012 at 12:59:55AM +0200, Marko Kreen wrote:
On Fri, Dec 21, 2012 at 10:27 PM, Noah Misch n...@leadboat.com wrote:
This should have gone to secur...@postgresql.org, instead.
It went and this could have been patched in 9.2.2 but they did not care.
Ah.
On Fri, Dec 21, 2012 at 06:05:10PM +0200, Marko Kreen wrote:
When there is 'ssl=on' then postmaster calls SSL_CTX_new(),
which asks for random number, thus requiring initialization
of randomness pool (RAND_poll). After that all forked backends
think pool is already initialized. Thus they proceed with same
fixed state they got from postmaster.
Attached patch makes both gen_random_bytes() and pgp_encrypt()
seed pool with output from gettimeofday(), thus getting pool
off from fixed state. Basically, this mirrors what SSL_accept()
already does.
That adds only 10-20 bits of entropy. Is that enough?
It's enough to get numbers that are not the same.
I think I see what you're getting at. With an ssl=on postmaster that only
processes non-SSL connections, calls to gen_random_bytes() in a session can,
at least initially, only produce about 2^15[1] distinct random sequences per
postmaster restart. Furthermore, an attacker with access to run queries can
generate the list of possible sequences, then proceed to know the exact bytes
each other backend with receive, for the life of the postmaster. With your
patch, no two backends will ever mix the same PID+microsecond time into the
random state.
For anyone following along, the following Perl program demonstrates the
weakness. After it has visited the full range of PIDs, every future iteration
will find a duplicate:
for my $offset (1 .. 10) {
my $db = DBI-connect('dbi:Pg:', undef, undef, {RaiseError = 1});
my($bytes) = $db-selectrow_array('SELECT gen_random_bytes(10)');
print Saw $bytes twice!\n if $seen{$bytes};
$seen{$bytes}++;
print Done $offset\n unless $offset % 100;
}
Whether it's possible to guess next number when you know
that there is only time() and getpid() added to fixed state
(eg. those cleartext bytes in SSL handshake) - i don't know.
I don't know, either.
In any case, this is how Postgres already operates SSL connections.
Fair point. Interesting reading (search for fork):
http://www.acsac.org/2003/papers/79.pdf
http://www.apache-ssl.org/randomness.pdf
http://openssl.6102.n7.nabble.com/recycled-pids-causes-PRNG-to-repeat-td41669.html
The first paper suggests exactly what you've implemented. Between that and
OpenSSL doing it, I don't feel further need to doubt its adequacy.
How about instead calling RAND_cleanup() after each backend fork?
Not instead - the gettimeofday() makes sense in any case. Considering
that it's immediate problem only for pgcrypto, this patch has higher chance
for appearing in back-branches.
I worry that it's too cavalier to leave the OpenSSL PRNG in an insecure state,
expecting every consumer to fix the problem or, failing to do so, silently
operate in an insecure manner. pgcrypto may be the only security-critical
user in our tree, but others in the field would not surprise me. Putting the
change after fork_process() in BackendStartup() solves the problem for good.
If the _cleanup() makes next RAND_bytes() call RAND_poll() again?
Then yes, it certainly makes sense as core patch.
It does. The disadvantage is that we'll then draw bytes from /dev/urandom for
every backend fork. The Linux /dev/urandom is mostly designed for such abuse,
but it would carry some unnecessary risk for back branches. So, I think your
mixing of time into the seed is indeed best.
Thanks,
nm
[1] That assumes the common 1 ... 32768 PID range.
--
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] Commits 8de72b and 5457a1 (COPY FREEZE)
On Sat, Dec 22, 2012 at 12:42:43AM +, Simon Riggs wrote: On 21 December 2012 20:10, Noah Misch n...@leadboat.com wrote: I thought of one case where we do currently forget rd_newRelfilenodeSubid: BEGIN; TRUNCATE t; SAVEPOINT save; TRUNCATE t; ROLLBACK TO save; That's a weird one. Aborting a subtransacton that sets it, when it was already set. The loss of rd_newRelfilenodeSubid in that case is deterministic, but tracking the full complexity of multiple relations and multiple nested subxids isn't worth the trouble for such rare cases [assumption]. I'd go for just setting an its_too_complex flag (with better name) that we can use to trigger a message in COPY to say that FREEZE option won't be honoured. That would then be completely consistent, rather than the lack of deterministic behaviour that Robert rightly objects to. I wouldn't bother. The behavior here is deterministic, the cause clearly traceable to the specific commands issued. Stable software won't suddenly miss the optimization for no visible reason. -- 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] Making view dump/restore safe at the column-alias level
Robert Haas robertmh...@gmail.com writes: I'm having a hard time following this. Can you provide a concrete example? regression=# create table t1 (x int, y int); CREATE TABLE regression=# create table t2 (x int, z int); CREATE TABLE regression=# create view v1 as select * from t1 join t2 using (x); CREATE VIEW regression=# \d+ v1 View public.v1 Column | Type | Modifiers | Storage | Description +-+---+-+- x | integer | | plain | y | integer | | plain | z | integer | | plain | View definition: SELECT t1.x, t1.y, t2.z FROM t1 JOIN t2 USING (x); regression=# alter table t2 rename column x to q; ALTER TABLE regression=# \d+ v1 View public.v1 Column | Type | Modifiers | Storage | Description +-+---+-+- x | integer | | plain | y | integer | | plain | z | integer | | plain | View definition: SELECT t1.x, t1.y, t2.z FROM t1 JOIN t2 USING (x); At this point the dumped view definition is wrong: if you try to execute it you get regression=# SELECT t1.x, t1.y, t2.z regression-#FROM t1 regression-#JOIN t2 USING (x); ERROR: column x specified in USING clause does not exist in right table I'm suggesting that we could fix this by emitting something that forces the right alias to be assigned to t2.q: SELECT t1.x, t1.y, t2.z FROM t1 JOIN t2 AS t2(x,z) USING (x); The implementation I have in mind is to recurse down the join tree and have any JOIN USING item forcibly propagate the common column name as the alias-to-use for each of the two input columns. Also consider regression=# create view v2 as select * from (select 1,2) as a(x,y) regression-# union select * from (select 3,4) as b; CREATE VIEW regression=# \d+ v2 View public.v2 Column | Type | Modifiers | Storage | Description +-+---+-+- x | integer | | plain | y | integer | | plain | View definition: SELECT a.x, a.y FROM ( SELECT 1, 2) a(x, y) UNION SELECT b.?column? AS x, b.?column? AS y FROM ( SELECT 3, 4) b; That view definition doesn't work either, as complained of today in pgsql-general. To fix this we just need to force the columns of b to be given distinct aliases. The minimum-new-code solution would probably be to produce SELECT a.x, a.y FROM ( SELECT 1, 2) a(x, y) UNION SELECT b.?column? AS x, b.?column?_1 AS y FROM ( SELECT 3, 4) b(?column?, ?column?_1) using the same add-some-digits-until-unique logic we are using for relation aliases. This could be done by considering all the column aliases of each RTE when we arrive at it during the recursive scan. On further reflection I think my worry about the top-level aliases was unfounded --- we prevent views from being created at all unless the top-level column names are all distinct. But we definitely have got issues for lower-level aliases, as these examples show. 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] pgcrypto seeding problem when ssl=on
Noah Misch n...@leadboat.com writes: On Sat, Dec 22, 2012 at 12:59:55AM +0200, Marko Kreen wrote: On Fri, Dec 21, 2012 at 10:27 PM, Noah Misch n...@leadboat.com wrote: This should have gone to secur...@postgresql.org, instead. It went and this could have been patched in 9.2.2 but they did not care. Ah. A slightly less revisionist view of the history is that the patch that Marko originally proposed to -security was considerably more complicated and less portable than this one, so we put it on hold until we thought of something better. This new patch looks pretty reasonable from here, modulo the question of whether it adds enough entropy. I'm not entirely sold on whether it does. ISTM that any attack based on this line of thinking already assumes that the attacker has complete knowledge of how many backends have been launched (else he doesn't know which sequence a targeted session will get). If he knows that much, mightn't he also know *when* they were launched? Alternatively: if he can know the session's start time (which we helpfully make available...) how much harder is this really making it for him to deduce the session's seed? On top of which: what exactly will he do with the seed once he's got it that would amount to a security problem? Or to put it in different terms, I'm not quite convinced that there's a plausible threat model that this patch blocks effectively. 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] pgcrypto seeding problem when ssl=on
On Fri, Dec 21, 2012 at 10:05:30PM -0500, Tom Lane wrote: This new patch looks pretty reasonable from here, modulo the question of whether it adds enough entropy. More brains reviewing that question will be good. I'm not entirely sold on whether it does. ISTM that any attack based on this line of thinking already assumes that the attacker has complete knowledge of how many backends have been launched (else he doesn't know which sequence a targeted session will get). pg_stat_activity.pid will do. If he knows that much, mightn't he also know *when* they were launched? Alternatively: if he can know the session's start time (which we helpfully make available...) how much harder is this really making it for him to deduce the session's seed? If he has access to his own fork-time seed, he can indeed estimate the seeds of other sessions. That seed is currently invisible to non-native code, and a user able to deploy a C function already has the access needed to compromise all sessions. The assumption that the fork-time seed stays buried is a source of unease, though. On top of which: what exactly will he do with the seed once he's got it that would amount to a security problem? Or to put it in different terms, I'm not quite convinced that there's a plausible threat model that this patch blocks effectively. The examples could be as numerous as the algorithms that specify use of a cryptographically secure PRNG. Here's a simple one: an application generates long-term private encryption keys by connecting, issuing SELECT gen_random_bytes(16), and disconnecting. Long-term, across postmaster restarts, there are still almost 2^128 possible keys. However, backends of any given postmaster can only generate 2^15 possible keys. An attacker can attempt to acquire, over time, a backend with every PID in the system. The script I gave earlier is an example of doing so. He won't manage to visit literally every PID, sure, but he'll easily get 95% of them. By issuing SELECT pg_backend_pid(), gen_random_bytes(16) in each session, he assembles a dictionary of most possible keys under this postmaster. If he repeats this after each postmaster restart, he might acquire a dictionary covering months or years of key generation. Given a ciphertext based on a key presumed to be made during that time, he can try his relatively-small dictionary with a high chance of success. nm -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_basebackup from cascading standby after timeline switch
On Friday, December 21, 2012 6:24 PM Heikki Linnakangas wrote: On 17.12.2012 18:58, Magnus Hagander wrote: On Mon, Dec 17, 2012 at 5:19 PM, Tom Lanet...@sss.pgh.pa.us wrote: Heikki Linnakangashlinnakan...@vmware.com writes: I'm not happy with the fact that we just ignore the problem in a backup taken from a standby, silently giving the user a backup that won't start up. Why not include the timeline history file in the backup? +1. I was not aware that we weren't doing that --- it seems pretty foolish, especially since as you say they're tiny. Yeah, +1. That should probably have been a part of the whole basebackup from slave patch, so it can probably be considered a back-patchable bugfix in itself, no? Yes, this should be backpatched to 9.2. I came up with the attached. One solution to that would be to pay more attention to the timelines to include WAL from. basebackup.c could read the timeline history file, to see exactly where the timeline switches happened, and then construct the filename of each WAL segment using the correct timeline id. Another approach would be to do readdir() on pg_xlog, and include all WAL files, regardless of timeline IDs, that fall in the right XLogRecPtr range. The latter seems easier to backpatch. I also think approach implemented by you is more better. One small point, shouldn't it check (walsender_shutdown_requested || walsender_ready_to_stop) during ReadDir of pg_xlog similar to what is done in ReadDir() in SendDir? With Regards, Amit Kapila. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review of Row Level Security
2012/12/21 Stephen Frost sfr...@snowman.net: It seems to me we need some more discussion about design and implementation on row-security checks of writer-side, to reach our consensus. Again, I agree with Kevin on this- there should be a wiki or similar which actually outlines the high-level design, syntax, etc. That would help us understand and be able to meaningfully comment about the approach. I also. RLS entry of wiki has not been updated for long time, I'll try to update the entry for high-level design in a couple of days. On the other hand, we are standing next to the consensus about reader-side; a unique row-security policy (so, first version does not support per-command policy) shall be checked on table scanning on select, update or delete commands. I don't feel that we've really reached a consensus about the 'reader-side' implemented in this patch- rather, we've agreed (at a pretty high level) what the default impact of RLS for SELECT queries is. While I'm glad that we were able to do that, I'm rather dismayed that it took a great deal of discussion to get to that point. Thanks, Stephen -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review of Row Level Security
2012/12/22 Simon Riggs si...@2ndquadrant.com: On 21 December 2012 22:01, Stephen Frost sfr...@snowman.net wrote: On the other hand, we are standing next to the consensus about reader-side; a unique row-security policy (so, first version does not support per-command policy) shall be checked on table scanning on select, update or delete commands. I don't feel that we've really reached a consensus about the 'reader-side' implemented in this patch- rather, we've agreed (at a pretty high level) what the default impact of RLS for SELECT queries is. While I'm glad that we were able to do that, I'm rather dismayed that it took a great deal of discussion to get to that point. Would anybody like to discuss this on a conference call on say 28th Dec, to see if we can agree a way forwards? I feel certain that we can work through any difficulties and agree a minimal subset for change. All comers welcome, just contact me offlist for details. Of course, I'll join the conference. Please give me the detail. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
