Re: [HACKERS] Something flaky in the "relfilenode mapping" infrastructure
Andres Freund writes: > On 2014-06-12 00:38:36 -0400, Noah Misch wrote: >> http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prairiedog&dt=2014-06-12%2000%3A17%3A07 > Hm. My guess it's that it's just a 'harmless' concurrency issue. The > test currently run in concurrency with others: I think what happens is > that the table gets dropped in the other relation after the query has > acquired the mvcc snapshot (used for the pg_class) test. > But why is it triggering on such a 'unusual' system and not on others? > That's what worries me a bit. prairiedog is pretty damn slow by modern standards. OTOH, I think it is not the slowest machine in the buildfarm; hamster for instance seems to be at least a factor of 2 slower. So I'm not sure whether to believe it's just a timing issue. 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] Something flaky in the "relfilenode mapping" infrastructure
On 2014-06-12 00:38:36 -0400, Noah Misch wrote: > On Tue, Apr 15, 2014 at 03:28:41PM -0400, Tom Lane wrote: > > Andres Freund writes: > > >> On 2014-03-27 08:02:35 -0400, Tom Lane wrote: > > >>> Buildfarm member prairiedog thinks there's something unreliable about > > >>> commit f01d1ae3a104019d6d68aeff85c4816a275130b3: > > > > > So I had made a notice to recheck on > > > this. > > > http://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=prairiedog&br=HEAD > > > indicates there haven't been any further failures... So, for now I > > > assume this was caused by some problem fixed elsewhere. > > > > Hard to say. In any case, I agree we can't make any progress unless we > > see it again. > > The improved test just tripped: Hrmpf. Just one of these days I was happy thinking it was gone... > http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prairiedog&dt=2014-06-12%2000%3A17%3A07 Hm. My guess it's that it's just a 'harmless' concurrency issue. The test currently run in concurrency with others: I think what happens is that the table gets dropped in the other relation after the query has acquired the mvcc snapshot (used for the pg_class) test. But why is it triggering on such a 'unusual' system and not on others? That's what worries me a bit. 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] lo_create(oid, bytea) breaks every extant release of libpq
2014-06-12 7:08 GMT+02:00 Tom Lane : > Noah Misch writes: > > On Wed, Jun 11, 2014 at 11:57:20PM -0400, Tom Lane wrote: > >> Meanwhile, we have to either revert the addition of lo_create(oid, > >> bytea) altogether, or choose a different name for it. Suggestions? > > > lo_new() or lo_make()? An earlier draft of the patch that added > > lo_create(oid, bytea) had a similar function named make_lo(). > > I think we want to stick to the lo_xxx naming convention, whatever > xxx ends up being. > > I was idly thinking that we might want to focus on the fact that this > function not only creates a LO but loads some data into it. lo_make > isn't too bad, but we could also consider lo_load, lo_import, etc. > (lo_import is not one of the names we have to avoid overloading. > OTOH, there's already a 2-argument form of it, so maybe there'd be > issues with resolving calls with unknown-literal arguments.) > > I have not any problem with lo_new, lo_make. lo_import is related to import from host system. I am not sure about lo_load, but I am not able to specify arguments why not. Pavel > regards, tom lane > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
Re: [HACKERS] Something flaky in the "relfilenode mapping" infrastructure
Noah Misch writes: > The improved test just tripped: > http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prairiedog&dt=2014-06-12%2000%3A17%3A07 Ugh. If the MTBF is circa three months, how will we catch this before we're dead? A quick look around the machine's buildfarm directory says there's nothing left behind that's not included in the buildfarm server report, so I can't offer any immediate insight. But I can certainly load it up running some additional tests if anyone has an idea what to look for. 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] lo_create(oid, bytea) breaks every extant release of libpq
2014-06-12 6:54 GMT+02:00 Noah Misch : > On Wed, Jun 11, 2014 at 11:57:20PM -0400, Tom Lane wrote: > > While there's certainly a good argument to be made for making > > lo_initialize do that query differently, there's no way that we > > can fix every copy of libpq that's in the field. I think we have to > > consider that "there can be only one lo_create" is effectively part of > > the protocol spec for the foreseeable future. (It'd be easy enough > > to add a check in the opr_sanity regression test to catch violations > > of this rule.) > > > > It's also extremely unfortunate that the regression tests don't > > create (or at least don't leave behind) any large objects, as we > > might then have possibly caught this bug much earlier. > > Agreed. > > > Meanwhile, we have to either revert the addition of lo_create(oid, > > bytea) altogether, or choose a different name for it. Suggestions? > > lo_new() or lo_make()? An earlier draft of the patch that added > lo_create(oid, bytea) had a similar function named make_lo(). > +1 for lo_new Regards Pavel > > Thanks, > nm > > -- > Noah Misch > EnterpriseDB http://www.enterprisedb.com > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
Re: [HACKERS] lo_create(oid, bytea) breaks every extant release of libpq
Noah Misch writes: > On Wed, Jun 11, 2014 at 11:57:20PM -0400, Tom Lane wrote: >> Meanwhile, we have to either revert the addition of lo_create(oid, >> bytea) altogether, or choose a different name for it. Suggestions? > lo_new() or lo_make()? An earlier draft of the patch that added > lo_create(oid, bytea) had a similar function named make_lo(). I think we want to stick to the lo_xxx naming convention, whatever xxx ends up being. I was idly thinking that we might want to focus on the fact that this function not only creates a LO but loads some data into it. lo_make isn't too bad, but we could also consider lo_load, lo_import, etc. (lo_import is not one of the names we have to avoid overloading. OTOH, there's already a 2-argument form of it, so maybe there'd be issues with resolving calls with unknown-literal arguments.) 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] lo_create(oid, bytea) breaks every extant release of libpq
2014-06-12 6:22 GMT+02:00 Tatsuo Ishii : > > Meanwhile, we have to either revert the addition of lo_create(oid, > > bytea) altogether, or choose a different name for it. Suggestions? > > I wonder if there's any use case where we need to store bytea into > large objects. Don't we already have bytea data type? If the use case > is for large data which does not fit in a tuple, I am afraid that the > query string could become extremely big one. > I know a one use case - and I used it in my one application. For one my customer I wrote a application that ensures a data change between two independent subjects based on XML. It was relative simple architecture - applications solved a communication, and stored procedures solved a content - good success was a using of SQL/XML. After few years the communication protocol was enhanced about attached tiff scans - serialized in base64 in result XML doc. I had to quickly fix a this application with minimal impacts to others applications. And LO API is perfect for transporting binary data from/to database. But next I needed a functions for conversion between bytea and LO. Regards Pavel > > Best regards, > -- > Tatsuo Ishii > SRA OSS, Inc. Japan > English: http://www.sraoss.co.jp/index_en.php > Japanese:http://www.sraoss.co.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] lo_create(oid, bytea) breaks every extant release of libpq
On Wed, Jun 11, 2014 at 11:57:20PM -0400, Tom Lane wrote: > While there's certainly a good argument to be made for making > lo_initialize do that query differently, there's no way that we > can fix every copy of libpq that's in the field. I think we have to > consider that "there can be only one lo_create" is effectively part of > the protocol spec for the foreseeable future. (It'd be easy enough > to add a check in the opr_sanity regression test to catch violations > of this rule.) > > It's also extremely unfortunate that the regression tests don't > create (or at least don't leave behind) any large objects, as we > might then have possibly caught this bug much earlier. Agreed. > Meanwhile, we have to either revert the addition of lo_create(oid, > bytea) altogether, or choose a different name for it. Suggestions? lo_new() or lo_make()? An earlier draft of the patch that added lo_create(oid, bytea) had a similar function named make_lo(). Thanks, nm -- Noah Misch EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Something flaky in the "relfilenode mapping" infrastructure
On Tue, Apr 15, 2014 at 03:28:41PM -0400, Tom Lane wrote: > Andres Freund writes: > >> On 2014-03-27 08:02:35 -0400, Tom Lane wrote: > >>> Buildfarm member prairiedog thinks there's something unreliable about > >>> commit f01d1ae3a104019d6d68aeff85c4816a275130b3: > > > So I had made a notice to recheck on > > this. > > http://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=prairiedog&br=HEAD > > indicates there haven't been any further failures... So, for now I > > assume this was caused by some problem fixed elsewhere. > > Hard to say. In any case, I agree we can't make any progress unless we > see it again. The improved test just tripped: http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prairiedog&dt=2014-06-12%2000%3A17%3A07 -- Noah Misch EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] lo_create(oid, bytea) breaks every extant release of libpq
> Meanwhile, we have to either revert the addition of lo_create(oid, > bytea) altogether, or choose a different name for it. Suggestions? I wonder if there's any use case where we need to store bytea into large objects. Don't we already have bytea data type? If the use case is for large data which does not fit in a tuple, I am afraid that the query string could become extremely big one. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.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] Proposing pg_hibernate
On Wed, Jun 11, 2014 at 10:56 AM, Robert Haas wrote: > On Tue, Jun 10, 2014 at 10:03 PM, Gurjeet Singh wrote: >> And it's probably accepted by now that such a bahviour is not >> catastrophic, merely inconvenient. > > I think the whole argument for having pg_hibernator is that getting > the block cache properly initialized is important. If it's not > important, then we don't need pg_hibernator in the first place. But > if it is important, then I think not loading unrelated blocks into > shared_buffers is also important. I was constructing a contrived scenario, something that would rarely happen in reality. I feel that the benefits of this feature greatly outweigh the minor performance loss caused in such an unlikely scenario. Best regards, -- Gurjeet Singh http://gurjeet.singh.im/ EDB www.EnterpriseDB.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposing pg_hibernate
On Wed, Jun 11, 2014 at 12:25 AM, Amit Kapila wrote: > On Wed, Jun 11, 2014 at 7:59 AM, Gurjeet Singh wrote: >> On Sun, Jun 8, 2014 at 3:24 AM, Amit Kapila >> wrote: >> > Yeap, but if it crashes before writing checkpoint record, it will lead >> > to >> > recovery which is what we are considering. >> >> Good point. >> >> In case of such recovery, the recovery process will read in the blocks >> that were recently modified, and were possibly still in shared-buffers >> when Checkpointer crashed. So after recovery finishes, the >> BlockReaders will be invoked (because save-files were successfully >> written before the crash), and they would request the same blocks to >> be restored. Most likely, those blocks would already be in >> shared-buffers, hence no cause of concern regarding BlockReaders >> evicting buffers populated by recovery. > > Not necessarily because after crash, recovery has to start from > previous checkpoint, so it might not perform operations on same > pages as are saved by buffer saver. Granted, the recovery may not start that way (that is, reading in blocks that were in shared-buffers when shutdown was initiated), but it sure would end that way. Towards the end of recovery, the blocks it'd read back in are highly likely to be the ones that were present in shared-buffers at the time of shutdown. By the end of recovery, either (a) blocks read in at the beginning of recovery are evicted by later operations of recovery, or (b) they are still present in shared-buffers. So the blocks requested by the BlockReaders are highly likely to be already in shared-buffers at the end of recovery, because these are the same blocks that were dirty (and hence recorded in WAL) just before shutdown time. I guess what I am trying to say is that the blocks read in by the BlockReaders will be a superset of those read in by the reocvery process. At the time of shutdown/saving-buffers, the shared-buffers may have contained dirty and clean buffers. WAL contains the info of which blocks were dirtied. Recovery will read back the blocks that were dirty, to replay the WAL, and since the BlockReaders are started _after_ recovery finishes, the BlockReaders will effectively read in only those blocks that are not already read-in by the recovery. I am not yet convinced, at least in this case, that Postgres Hibernator would restore blocks that can cause eviction of buffers restored by recovery. I don't have intimate knowledge of recovery but I think the above assessment of recovery's operations holds true. If you still think this is a concern, can you please provide a bit firm example using which I can visualize the problem you're talking about. > Also as the file saved by > buffer saver can be a file which contains only partial list of > buffers which were in shared buffer's, it becomes more likely that > in such cases it can override the buffers populated by recovery. I beg to differ. As described above, the blocks read-in by the BlockReader will not evict the recovery-restored blocks. The save-files being written partially does not change that. > Now as pg_hibernator doesn't give any preference to usage_count while > saving buffer's, it can also evict the buffers populated by recovery > with some lower used pages of previous run. The case we're discussing (checkpointer/BufferSaver/some-other-process crash during a smart/fast shutdown) should occur rarely in practice. Although Postgres Hibernator is not yet proven to do the wrong thing in this case, I hope you'd agree that BlockReaders evicting buffers populated by recovery process is not catastrophic at all, merely inconvenient from performance perspective. Also, the impact is only on the initial performance immediately after startup, since application queries will re-prime the shared-buffers with whatever buffers they need. > I think Block Readers will remove file only after reading and populating > buffers from it Correct. > and that's the reason I mentioned that it can lead to doing > both recovery as well as load buffers based on file saved by buffer > saver. I am not sure I completely understand the implication here, but I think the above description of case where recovery-followed-by-BlockReaders not causing a concern may cover it. Best regards, -- Gurjeet Singh http://gurjeet.singh.im/ EDB www.EnterpriseDB.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] lo_create(oid, bytea) breaks every extant release of libpq
While investigating a different issue, I was astonished to find that pg_restore in HEAD is incapable of restoring dumps containing large objects: it fails with messages like pg_restore: [archiver] could not create large object 10: ERROR: function call message contains 1 arguments but function requires 2 After some investigation, I find that: 1. Commit c50b7c09d added a function lo_create(oid, bytea). 2. libpq's lo_initialize function, in fe-lobj.c, assumes that the pg_catalog schema will contain *only one* function named lo_create (and likewise for lo_open and a dozen other names). With more than one lo_create function, it's luck of the draw which one's OID ends up in the PGlobjfuncs struct. If it's the wrong one, subsequent attempts to use libpq's lo_create() function fail as above. While there's certainly a good argument to be made for making lo_initialize do that query differently, there's no way that we can fix every copy of libpq that's in the field. I think we have to consider that "there can be only one lo_create" is effectively part of the protocol spec for the foreseeable future. (It'd be easy enough to add a check in the opr_sanity regression test to catch violations of this rule.) It's also extremely unfortunate that the regression tests don't create (or at least don't leave behind) any large objects, as we might then have possibly caught this bug much earlier. Meanwhile, we have to either revert the addition of lo_create(oid, bytea) altogether, or choose a different name for it. Suggestions? 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] postgresql.auto.conf read from wrong directory
On Tue, May 27, 2014 at 10:35 AM, Amit Kapila wrote: > On Sun, May 11, 2014 at 11:23 PM, Tom Lane wrote: > > I think it's clearly *necessary* to forbid setting data_directory in > > postgresql.auto.conf. The file is defined to be found in the data > > directory, so any such setting is circular logic by definition; > > no good can come of not rejecting it. > > > > We already have a GUC flag bit about disallowing certain variables > > in the config file (though I don't remember if it's enforced or > > just advisory). It seems to me that we'd better invent one for > > disallowing in ALTER SYSTEM, as well. > > I introduced a new flag bit (GUC_DISALLOW_IN_AUTO_FILE) to > disallow parameters by Alter System and disallowed data_directory to > be set by Alter System. I have added this patch in next CF to avoid getting it lost. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
[HACKERS] Few observations in replication slots related code
Few observations in Replication slots related code:
1. In function StartupReplicationSlots(XLogRecPtr checkPointRedo),
parameter checkPointRedo is not used.
2. Few check are in different order in functions
pg_create_physical_replication_slot() and
pg_create_logical_replication_slot().
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
check_permissions();
CheckLogicalDecodingRequirements();
I don't think it makes any difference, however having checks in similar
order seems better unless there is a reason for not doing so.
3. Currently there is any Assert (Assert(!MyReplicationSlot);) in
pg_create_logical_replication_slot(), is there a need for similar
Assert in pg_create_physical_replication_slot()?
4.
StartupDecodingContext()
{
..
context = AllocSetContextCreate(CurrentMemoryContext,
"Changeset Extraction Context",
}
To be consistent, shall we name this context as
logical | logical decoding?
5.
pg_create_logical_replication_slot()
{
..
CreateInitDecodingContext()
..
ReplicationSlotPersist()
}
Function pg_create_logical_replication_slot() is trying to
save slot twice once during CreateInitDecodingContext() and
then in ReplicationSlotPersist(), isn't it better if we can make
it do it just once?
6.
elog(ERROR, "cannot handle changeset extraction yet");
Shouldn't it be better to use logical replication instead
of changeset extraction?
7.
+ * XXX: It might make sense to introduce ephemeral slots and always use
+ * the slot mechanism.
Already there are RS_EPHEMERAL slots, might be this
comment needs bit of rephrasing.
8. Is there a chance of inconsistency, if pg_restxlog resets the
xlog and after restart, one of the slots contains xlog position
older than what is resetted by pg_resetxlog?
9.
void
LogicalIncreaseXminForSlot(XLogRecPtr current_lsn, TransactionId xmin)
{
..
/*
* don't overwrite if we already have a newer xmin. This can happen if we
* restart decoding in a slot.
*/
if (TransactionIdPrecedesOrEquals(xmin, slot->data.catalog_xmin))
{
}
..
}
Should we just release spinlock in this loop and just return,
rather than keeping no action loop?
10.
* Iff ri_type = REPLICA_IDENTITY_INDEX, indexOid must be the Oid of a
suitable
* index. Otherwise, it should be InvalidOid.
*/
static void
relation_mark_replica_identity(Relation rel, char ri_type, Oid indexOid,
bool is_internal)
typo - *Iff*
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] API change advice: Passing plan invalidation info from the rewriter into the planner?
On Wed, Jun 11, 2014 at 8:59 PM, Stephen Frost wrote: >> Row-level security is not a chance for the system to deny access; it's >> a chance for user-defined code to take control and perform arbitrary >> operations. So the scope of what we're contemplating for row-level >> security is really far, far more invasive than what we did for >> column-level privileges. > > In this case the user-defined code needs to return a boolean. We don't > currently do anything to prevent it from having side-effects, no, but > the same is true with views which incorporate functions. I agree that > it makes a difference when compared to column-level privileges, but my > point was that we have provided easier ways to do things which were > possible using more complicated methods before. Perhaps the risk with > RLS is higher but these issues look managable to me and the level of > doubt about our ability to provide this feature in a reasonable and > principled way that our users will understand surprises me. I'm glad the issues look manageable to you, but you haven't really explained how to manage them. The way to dispel doubt is to come up with specific technical proposals that address the technical issues that have been raised. I accept that you are surprised that someone might not think we are on the right course here, but it's entirely appropriate for me to express my doubts about this or any other patch, much as many people do in regards to many patches that are posted here - generally for good and valid reasons. For my part, I'm mildly surprised that anyone thinks it's a good idea to have SELECT * FROM tab to mean different things depending on who is typing it. To me, that seems very confusing; how does an unprivileged user with no ability to assume some other role validate that the row security policy they've configured works at all and exposes precisely the intended set of rows? Even aside from security exposures, how does a non-superuser who runs pg_dump know whether they've got a complete backup or a filtered dump that's missing some rows? A filtered dump might not even be restorable if foreign keys are involved. I think those are serious issues that deserve serious thought and consideration, not just a vague assurance that the issues are probably manageable. >> Because we don't have a good design. > > We're using a design that's found in multiple other RDBMS's and used > extensively by certain industries which use those RDBMS's today. I'm > certainly open to improving what is found in other systems for PG but I > have a hard time seeing this approach as a bad design. Perhaps you're > referring to our implementation, in which case I might agree and things > like running the quals as the table owner is something which should be > considered (I don't know how the other RDBMS's operate in this regard > offhand- it'd be good to find out). I'm not referring to the proposed implementation particularly; or at least not that aspect of it. I don't think trying to run the view quals as the defining user is likely to be very appealing, because I think it's going to hurt performance, for example by preventing function inlining and requiring lots of user-ID switches. But I'm not gonna complain if someone wants to mull it over and make a proposal for how to make it work. Rather, my concern is that all we've got is what might be called the core of the feature; the actual guts of it. There are a lot of ancillary details that seem to me to be not worked out at all yet, or only half-baked. >> I'm not categorically opposed to adding more RLS features to >> PostgreSQL and never have been; in fact, I was deeply involved in the >> original design of security barrier views and committed the original >> patch to add that functionality to PostgreSQL, without which none of >> what we're talking about here would be possible. But the >> currently-proposed design is very unappealing to me, for the reasons >> that I've explained. The right answer to "this feature doesn't >> provide anything that we don't already have and will introduce major >> new security exposures that haven't been adequate thought" is >> debatable, but "well other people have this so we should too" is >> definitely not it. > > How about "it's in high demand by our user base"? In particular, it's > being asked for by a *highly* technical section of our user base who > uses these capabilities today, with this design, in those other > databases. Sure, that's a valid reason for considering any feature. But it's not an excuse to overlook whatever design problems may exist. >> Are you equally unimpressed with the idea that RLS as proposed can't >> support more than one security policy right now *at all*? Because it >> seems to me that either you think multiple RLS policies on a single >> table is important (in which case the current patch is inadequate) or >> you think it's not important (in which case we need not argue about >> whether doing it with multi
Re: [HACKERS] wrapping in extended mode doesn't work well with default pager
Noah Misch writes: > Based on the commit message and procedural history, I thought commit 6513633 > was changing behavior solely for the combination of "\pset expanded" and > "\pset format wrapped". Peter's and my test cases show that it also changed > behavior for "\pset expanded" alone. That's a bug, unless someone sees to > argue that the new "\pset expanded" behavior is a desirable improvement in > spite of its origin as an accident. Altering an entrenched psql output format > is a big deal. TBH I'm wondering if we shouldn't just revert that patch (and the subsequent fix attempts). It was not a major feature and I'm thinking we have better things to do right now than try to fix the multiple logic holes it evidently has. The author's certainly welcome to try again with a more carefully thought-through patch for 9.5. 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] wrapping in extended mode doesn't work well with default pager
On Wed, Jun 11, 2014 at 08:59:34PM +0100, Greg Stark wrote:
> The leading space that you (ie Peter) are complaining about in:
>
> col | 1
> +--
> col | 2
>
> Is there because if the cell wrapped it would get an ellipsis (ie
> '...' but it's a single unicode character) in that column to indicate
> that it's wrapped. However we don't wrap headers so the only reason to
> change it is for the "old-ascii" linestyle:
>
> stark=***# select * from (values (1),(2)) as _ ("col
> col");
> stark"***#
> col | 1
> +col ;
> -+-
> col | 2
> +col ;
>
> Noah's complaint is about the space padding on the *right*. Ie
>
> stark=***# select * from (values ('foo'),('foo bar baz')) as _ ("col");
> col | foo
> <- This is the end of the line
> -+--<-
> This is the end of the line
> col | foo bar baz
> <- This is the end of the line
>
> We didn't used to do that in expanded and I kind of agree it would be
> nice to avoid.
Based on the commit message and procedural history, I thought commit 6513633
was changing behavior solely for the combination of "\pset expanded" and
"\pset format wrapped". Peter's and my test cases show that it also changed
behavior for "\pset expanded" alone. That's a bug, unless someone sees to
argue that the new "\pset expanded" behavior is a desirable improvement in
spite of its origin as an accident. Altering an entrenched psql output format
is a big deal.
--
Noah Misch
EnterpriseDB http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] API change advice: Passing plan invalidation info from the rewriter into the planner?
* Robert Haas (robertmh...@gmail.com) wrote: > On Wed, Jun 11, 2014 at 12:23 PM, Stephen Frost wrote: > > This argument could have been made for column-level privileges also, no? > > Not really. First of all, we didn't have security_barrier views at > that time, let alone security barrier views that are auto-updateable. We had security definer functions, even set-returning ones, along with rules and triggers. > That's a really important piece of technology which makes filtering > access via views feasible in ways that really were not feasible in the > past. Secondly, column-level permissions - like every other > currently-existing type of permissions - are declarative. They are an > additional opportunity for the system to say "no" to something it > otherwise would have allowed, but no user-defined code is executed. We could try to avoid calling user-defined code for RLS, but it'd add a whole lot of complexity and as far as I can see and your proposed solution isn't avoiding the user-defined code anyway, so I'm not sure why this solution should be required to meet that. > Row-level security is not a chance for the system to deny access; it's > a chance for user-defined code to take control and perform arbitrary > operations. So the scope of what we're contemplating for row-level > security is really far, far more invasive than what we did for > column-level privileges. In this case the user-defined code needs to return a boolean. We don't currently do anything to prevent it from having side-effects, no, but the same is true with views which incorporate functions. I agree that it makes a difference when compared to column-level privileges, but my point was that we have provided easier ways to do things which were possible using more complicated methods before. Perhaps the risk with RLS is higher but these issues look managable to me and the level of doubt about our ability to provide this feature in a reasonable and principled way that our users will understand surprises me. > > I agree that views, or even security-definer functions, offer a great > > deal of flexibility, and that may be necessary in some use-cases, but I > > fail to see why that means we should avoid providing the mechanics to > > achieve simple and usable RLS akin to what other major RDBMS's have. > > Because we don't have a good design. We're using a design that's found in multiple other RDBMS's and used extensively by certain industries which use those RDBMS's today. I'm certainly open to improving what is found in other systems for PG but I have a hard time seeing this approach as a bad design. Perhaps you're referring to our implementation, in which case I might agree and things like running the quals as the table owner is something which should be considered (I don't know how the other RDBMS's operate in this regard offhand- it'd be good to find out). > I'm not categorically opposed to adding more RLS features to > PostgreSQL and never have been; in fact, I was deeply involved in the > original design of security barrier views and committed the original > patch to add that functionality to PostgreSQL, without which none of > what we're talking about here would be possible. But the > currently-proposed design is very unappealing to me, for the reasons > that I've explained. The right answer to "this feature doesn't > provide anything that we don't already have and will introduce major > new security exposures that haven't been adequate thought" is > debatable, but "well other people have this so we should too" is > definitely not it. How about "it's in high demand by our user base"? In particular, it's being asked for by a *highly* technical section of our user base who uses these capabilities today, with this design, in those other databases. > Craig's patch really hasn't grappled with any of > these thorny definition and security issues; it's just about making > the basic functionality work. That's fine for a POC, but it's not > enough for a feature that the project would be committing to maintain > for the indefinite future. Improving the patch is exactly what I'd like to do, but throwing out the notion that RLS can't be allowed to execute user-defined code is cutting the legs out of the feature completely- particularly with our system where users can create all manner of objects in the system with their own code being run. > >> That's mighty useful for debugging, at least IMHO. And, if you want > >> to have several row-level security policies for different classes of > >> users, just create more than one view and grant different privileges > >> on each. > > > > I'm really not impressed with the idea that RLS should be done with > > multiple different views of the same underlying table. > > Are you equally unimpressed with the idea that RLS as proposed can't > support more than one security policy right now *at all*? Because it > seems to me that either you think multiple RLS policies on a single > ta
Re: [HACKERS] [patch] build issues on Win32
On Wed, Mar 10, 2010 at 10:51:23AM -0500, Tom Lane wrote:
> =?utf-8?Q?Dag-Erling_Sm=C3=B8rgrav?= writes:
> > -LDFLAGS="-Wl,--allow-multiple-definition"
> > +LDFLAGS="${LDFLAGS} -Wl,--allow-multiple-definition"
>
> That bit seems sane.
I've committed that part. Thanks.
--
Noah Misch
EnterpriseDB http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] API change advice: Passing plan invalidation info from the rewriter into the planner?
Tom, * Tom Lane (t...@sss.pgh.pa.us) wrote: > Stephen Frost writes: > > * Robert Haas (robertmh...@gmail.com) wrote: > >> I'm really concerned about the security implications of this patch. I > >> think we're setting ourselves up for a whole lot of hurt for somewhat > >> unclear gain. > > > I'm certainly of a different opinion and, for the most part, I feel that > > if there are security concerns then they need to be addressed- and > > better by us than by asking users to use some other mechanism to > > implement RLS. > > TBH, I found Robert's argument pretty persuasive. The idea that > "SELECT * FROM table" might invoke arbitrary processing ought to scare > anyone who's concerned about security, because that's going to completely > break any assumptions about pg_dump being safe for instance, as well as > force top-to-bottom rethinking of many other security assumptions. SELECT triggers for a wide variety of use-cases are pretty commonly asked for here and are something I'd like to see us support also. There are also quite a few ways in which a select can end up executing code. Today it requires more than 'select * from table;', but not very much.. I agree that it'd be good if we had a way to address that but I continue to view that as an independent issue. What I haven't heard any comments on, yet found interesting, was the idea of having the RLS quals run as the owner of the table. Would that address these concerns? I seem to recall wondering why we didn't do that for views in the first place, though I doubt we could change it now even if we wanted to (and I'm guessing the spec has something to say about this, though I haven't gone and looked and don't remember offhand). It's certainly rather curious that functions called under a view are run as the calling user while permissions checks on relations referred to by the view are as the view owner. Hopefully that will make the rest of this discussion less relevant, but I'll respond with my feelings anyway. > > ... That commit was > > the ground-work to allow us to finally get proper RLS and I'm very > > disappointed to hear that the mechanical pieces around making RLS easy > > for users to use (and getting that check-box taken care of in a wide > > variety of fields that we are being exposed to now, see the PGConf.NYC > > keynote speakers...) is receiving such push-back. > > If this is being sold as merely "ease of use", then it is probably going > to get rejected. In order to get some extra ease of use for the minority > of users who need RLS, you are going to significantly complicate the lives > of all Postgres users. That's not a net win in any sane calculation of > ease of use. I don't view this as being at all accurate- how is this complicating the lives of all Postgres users? If they are worried about running user defined code then they *already* have a lot to worry about. While the users of RLS might be less than 50% and therefore the minority, I expect it will have quite a bit of up-take in certain industries and I know that our lack of any RLS is currently preventing use of Postgres in some rather important cases. As for it being ease-of-use, again, there are ways in which column level privileges could have been dealt with using views, rules, security definer functions, etc, but that doesn't mean we don't want that feature. I certainly view RLS (and have for quite some time..) as a much needed capability, even if it can be done today using a bunch of user written code that must be security audited. > Maybe the right thing to think about is how we can make it easier to set > up table + view combinations according to the pattern Robert described. While this sounds interesting, I don't see adding columns or redefining them as being in the perview of RLS. The current approach of allowing a boolean expression to be defined is both extremely flexible while also being simple when the requirement is simple. Having to create, manage, update, etc, an independent object would add unnecessary complexity. Perhaps having it be a boolean expression is too much flexibility but the alternatives that I can think of aren't terribly attractive to me and the boolean expression approach is what folks coming from other RDBMS's will be familiar with and understand how to build their applications around. We may need to provide some additional pieces around this (perhaps a trigger-like function type which also gets information about the object being queried, etc) but the point is to have a straight-forward and simply reasoned about way of limiting what data is returned. > I wouldn't have a problem with some more-or-less-automated support for > doing that. (Consider SERIAL as a possible precedent here: it's basically > a table creation macro.) Perhaps there's a way to make that work, but personally it looks like a whole bunch more work and I don't see the gain. How would adding RLS to an existing table work? It's worse than the SERIAL case
Re: [HACKERS] [GENERAL] Question about partial functional indexes and the query planner
Robert Haas writes: > On Tue, Jun 10, 2014 at 7:19 PM, Tom Lane wrote: >> Given the lack of previous complaints, I'm not sure this amounts to >> a back-patchable bug, but it does seem like something worth fixing >> going forward. > Agreed, although I'd be willing to see us slip it into 9.4. It's > doubtful that anyone will get upset if their query plans change > between beta1 and beta2, but the same cannot be said for released > branches. After further thought about this I realized that there's another category of proof possibilities that is pretty simple to add while we're at it. Namely, once we've found that both subexpressions of the two operator clauses are equal(), we can use btree opclass relationships to prove that, say, "x < y implies x <= y" or "x < y refutes x > y", independently of just what x and y are. (Well, they have to be immutable expressions, but we'd not get this far if they're not.) We already had pretty nearly all of the machinery for that, but again it was only used for proving cases involving comparisons to constants. A little bit of refactoring later, I offer the attached draft patch. I'm thinking this is probably more than we want to slip into 9.4 at this point, but I'll commit it to HEAD soon if there are not objections. regards, tom lane diff --git a/src/backend/optimizer/util/predtest.c b/src/backend/optimizer/util/predtest.c index 9d61a4d..039221f 100644 *** a/src/backend/optimizer/util/predtest.c --- b/src/backend/optimizer/util/predtest.c *** static bool predicate_refuted_by_simple_ *** 95,102 static Node *extract_not_arg(Node *clause); static Node *extract_strong_not_arg(Node *clause); static bool list_member_strip(List *list, Expr *datum); ! static bool btree_predicate_proof(Expr *predicate, Node *clause, ! bool refute_it); static Oid get_btree_test_op(Oid pred_op, Oid clause_op, bool refute_it); static void InvalidateOprProofCacheCallBack(Datum arg, int cacheid, uint32 hashvalue); --- 95,106 static Node *extract_not_arg(Node *clause); static Node *extract_strong_not_arg(Node *clause); static bool list_member_strip(List *list, Expr *datum); ! static bool operator_predicate_proof(Expr *predicate, Node *clause, ! bool refute_it); ! static bool operator_same_subexprs_proof(Oid pred_op, Oid clause_op, ! bool refute_it); ! static bool operator_same_subexprs_lookup(Oid pred_op, Oid clause_op, ! bool refute_it); static Oid get_btree_test_op(Oid pred_op, Oid clause_op, bool refute_it); static void InvalidateOprProofCacheCallBack(Datum arg, int cacheid, uint32 hashvalue); *** arrayexpr_cleanup_fn(PredIterInfo info) *** 1025,1032 * we are within an AND/OR subtree of a WHERE clause. (Again, "foo" is * already known immutable, so the clause will certainly always fail.) * ! * Finally, we may be able to deduce something using knowledge about btree ! * operator families; this is encapsulated in btree_predicate_proof(). *-- */ static bool --- 1029,1037 * we are within an AND/OR subtree of a WHERE clause. (Again, "foo" is * already known immutable, so the clause will certainly always fail.) * ! * Finally, if both clauses are binary operator expressions, we may be able ! * to prove something using the system's knowledge about operators; those ! * proof rules are encapsulated in operator_predicate_proof(). *-- */ static bool *** predicate_implied_by_simple_clause(Expr *** 1060,1067 return false; /* we can't succeed below... */ } ! /* Else try btree operator knowledge */ ! return btree_predicate_proof(predicate, clause, false); } /*-- --- 1065,1072 return false; /* we can't succeed below... */ } ! /* Else try operator-related knowledge */ ! return operator_predicate_proof(predicate, clause, false); } /*-- *** predicate_implied_by_simple_clause(Expr *** 1083,1090 * these cases is to support using IS NULL/IS NOT NULL as partition-defining * constraints.) * ! * Finally, we may be able to deduce something using knowledge about btree ! * operator families; this is encapsulated in btree_predicate_proof(). *-- */ static bool --- 1088,1096 * these cases is to support using IS NULL/IS NOT NULL as partition-defining * constraints.) * ! * Finally, if both clauses are binary operator expressions, we may be able ! * to prove something using the system's knowledge about operators; those ! * proof rules are encapsulated in operator_predicate_proof(). *-- */ static bool *** predicate_refuted_by_simple_clause(Expr *** 1148,1155 return false; /* we can't succeed below... */ } ! /* Else try btree operator knowledge */ ! return btree_predicate_proof(predicate, clause, true); } --- 1154,1161 return false; /* we can't su
Re: [HACKERS] view reloptions
On Wed, Jun 11, 2014 at 4:46 PM, Alvaro Herrera wrote: > I just noticed by chance that view relations are using StdRdOptions to > allocate their reloptions. I can't find any reason for this, other than > someone failed to realize that they should instead have a struct defined > of its own, just like (say) GIN indexes do. Views using StdRdOptions is > quite pointless, given that it's used for stuff like fillfactor and > autovacuum, neither of which apply to views. > > 9.2 added security_barrier to StdRdOptions, and 9.4 is now adding > check_option_offset, which is a string reloption with some funny rules. > > Is it too late to redefine the check_option_offset stuff before 9.4 > ships, so that it is in its own struct? (I'd hope we can redefine it in > a simpler way also, hopefully one that doesn't require strcmp()'ing that > string with "local" or "cascaded" every time someone is interested in > knowing the option's value for a particular view.) Are there a big problem with this implementation? I asked because we already do a strcmmp()'ing in 'buffering' option for GiST indexes since this commit http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=5edb24a8. Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL >> Timbira: http://www.timbira.com.br >> Blog sobre TI: http://fabriziomello.blogspot.com >> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello >> Twitter: http://twitter.com/fabriziomello
Re: [HACKERS] view reloptions
Alvaro Herrera writes: > Is it too late to redefine the check_option_offset stuff before 9.4 > ships, so that it is in its own struct? We have a forced initdb already for beta2, so I'd say not. > 3) I don't have time to do the legwork before CF1 anyway ... but if nobody does the work, it's moot. > If we don't change it now, I'm afraid we'll be stuck with using > StdRdOptions for views for all eternity. Why would we not be able to change it in 9.5? It's a catalog change, sure, but we'll no doubt have others. 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] wrapping in extended mode doesn't work well with default pager
And Gmail has thoroughly mangled that email. Let me see if I can resend it from Emacs more clearly. -- 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] wrapping in extended mode doesn't work well with default pager
On Wed, Jun 11, 2014 at 7:52 PM, Peter Eisentraut wrote:
> On 6/8/14, 11:29 PM, Noah Misch wrote:
>> The patch did not restore 9.3 behavior for that one. Starting with commit
>> 6513633, the first line of letters is space-padded on the right to the width
>> of the second line of letters. To illustrate, I have attached raw psql
>> output
>> from both commit 6513633 and its predecessor. Also note that
>> psql-wrapped-expanded-fix-v4.patch expands each [ RECORD x ] header from 509
>> bytes to 511 bytes; 509 is the longstanding width.
>
> I noticed that (or perhaps a related) problem today. Here is a simple demo:
I don't think these two issues are related.
The leading space that you (ie Peter) are complaining about in:
col | 1
+--
col | 2
Is there because if the cell wrapped it would get an ellipsis (ie
'...' but it's a single unicode character) in that column to indicate
that it's wrapped. However we don't wrap headers so the only reason to
change it is for the "old-ascii" linestyle:
stark=***# select * from (values (1),(2)) as _ ("col
col");
stark"***#
col | 1
+col ;
-+-
col | 2
+col ;
Noah's complaint is about the space padding on the *right*. Ie
stark=***# select * from (values ('foo'),('foo bar baz')) as _ ("col");
col | foo
<- This is the end of the line
-+--<-
This is the end of the line
col | foo bar baz
<- This is the end of the line
We didn't used to do that in expanded and I kind of agree it would be
nice to avoid. But then there are lots of cases where it would still
be necessary:
stark=***# select * from (values ('foo'),('foo bar
baz')) as _ ("col");
stark'***#
col | foo
<- This is the end of the line
-+--<-
This is the end of the line
col | foo bar
+<- This is the end of the line
| baz
<- This is the end of the line
Obviously we would need to space padd to insert the "+" there.
I think this whole exercise has mostly just convinced me we should
implement an HTTP interface and reimplement psql as a browser app.
--
greg
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] view reloptions
I just noticed by chance that view relations are using StdRdOptions to allocate their reloptions. I can't find any reason for this, other than someone failed to realize that they should instead have a struct defined of its own, just like (say) GIN indexes do. Views using StdRdOptions is quite pointless, given that it's used for stuff like fillfactor and autovacuum, neither of which apply to views. 9.2 added security_barrier to StdRdOptions, and 9.4 is now adding check_option_offset, which is a string reloption with some funny rules. Is it too late to redefine the check_option_offset stuff before 9.4 ships, so that it is in its own struct? (I'd hope we can redefine it in a simpler way also, hopefully one that doesn't require strcmp()'ing that string with "local" or "cascaded" every time someone is interested in knowing the option's value for a particular view.) There are some problems with this idea though, namely: 1) it's damn late in the release cycle already 2) it would mean that security_barrier would change for external code that expects StdRdOptions rather than, say, ViewOptions 3) I don't have time to do the legwork before CF1 anyway If we don't change it now, I'm afraid we'll be stuck with using StdRdOptions for views for all eternity. (It's a pity I didn't become aware of this earlier in 9.4 cycle when I added the multixact freeze reloptions ... I could have promoted a patch back then.) Thoughts? -- Álvaro Herrerahttp://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] API change advice: Passing plan invalidation info from the rewriter into the planner?
On Wed, Jun 11, 2014 at 12:23 PM, Stephen Frost wrote: >> In my view, commit 842faa714c0454d67e523f5a0b6df6500e9bc1a5 basically >> *is* row-level security: instead of applying a row-level security >> policy to a table, just create a security-barrier view over the table >> and grant access to the view. Forget that the table ever existed. >> Done. > > This argument could have been made for column-level privileges also, no? Not really. First of all, we didn't have security_barrier views at that time, let alone security barrier views that are auto-updateable. That's a really important piece of technology which makes filtering access via views feasible in ways that really were not feasible in the past. Secondly, column-level permissions - like every other currently-existing type of permissions - are declarative. They are an additional opportunity for the system to say "no" to something it otherwise would have allowed, but no user-defined code is executed. Row-level security is not a chance for the system to deny access; it's a chance for user-defined code to take control and perform arbitrary operations. So the scope of what we're contemplating for row-level security is really far, far more invasive than what we did for column-level privileges. > I agree that views, or even security-definer functions, offer a great > deal of flexibility, and that may be necessary in some use-cases, but I > fail to see why that means we should avoid providing the mechanics to > achieve simple and usable RLS akin to what other major RDBMS's have. Because we don't have a good design. I'm not categorically opposed to adding more RLS features to PostgreSQL and never have been; in fact, I was deeply involved in the original design of security barrier views and committed the original patch to add that functionality to PostgreSQL, without which none of what we're talking about here would be possible. But the currently-proposed design is very unappealing to me, for the reasons that I've explained. The right answer to "this feature doesn't provide anything that we don't already have and will introduce major new security exposures that haven't been adequate thought" is debatable, but "well other people have this so we should too" is definitely not it. Craig's patch really hasn't grappled with any of these thorny definition and security issues; it's just about making the basic functionality work. That's fine for a POC, but it's not enough for a feature that the project would be committing to maintain for the indefinite future. >> That's mighty useful for debugging, at least IMHO. And, if you want >> to have several row-level security policies for different classes of >> users, just create more than one view and grant different privileges >> on each. > > I'm really not impressed with the idea that RLS should be done with > multiple different views of the same underlying table. Are you equally unimpressed with the idea that RLS as proposed can't support more than one security policy right now *at all*? Because it seems to me that either you think multiple RLS policies on a single table is important (in which case the current patch is inadequate) or you think it's not important (in which case we need not argue about whether doing it with multiple views over the same underlying table is awkward). >> By contrast, it seems to me that every design so far proposed for >> something that is actually called row-level security - as opposed to >> commit 842faa714c0454d67e523f5a0b6df6500e9bc1a5, which *really is* >> row-level security, is extremely limited. Look back at all the things >> listed in the previous paragraph; can you do those things easily with >> the designs that have been proposed? As far as I can see, not really. > > I don't feel that RLS will, or even *should*, have the same level of > flexibility that you can achieve with views and/or security definer > functions. I expect that, over time, we will add more capabilities to > it, but it's never going to be able to redefine the contents of a column > as a view can, nor will it be able to add columns to a table as views > can. I don't see those as reasons against having support for RLS. What this patch is doing is basically allowing a table to really be a view over itself. If we choose to support that, I think it is absolutely inevitable that people are going to want all the same options that they would have if they really made a separate view - separate permissions, WITH CHECK OPTION, all of it. I find the contrary argument - that people will only want X amount and no more - simply not plausible. If it's valuable to have some of those capabilities in an RLS framework, somebody's going to want all of them. There's no bright line to divide the things that are valuable in that context from those that aren't. > I'm glad to hear your thoughts on the level of granularity which might > be nice to have with RLS. What would be great is to spend a bit more > time review
Re: [HACKERS] wrapping in extended mode doesn't work well with default pager
On 6/8/14, 11:29 PM, Noah Misch wrote: > The patch did not restore 9.3 behavior for that one. Starting with commit > 6513633, the first line of letters is space-padded on the right to the width > of the second line of letters. To illustrate, I have attached raw psql output > from both commit 6513633 and its predecessor. Also note that > psql-wrapped-expanded-fix-v4.patch expands each [ RECORD x ] header from 509 > bytes to 511 bytes; 509 is the longstanding width. I noticed that (or perhaps a related) problem today. Here is a simple demo: psql -X -q -t -x -c 'select * from (values (1),(2)) as _ (col)' 9.3: col | 1 +-- col | 2 9.4: col | 1 +-- col | 2 This breaks check_postgres. (Why check_postgres doesn't use unaligned output is beyond me.) -- 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] API change advice: Passing plan invalidation info from the rewriter into the planner?
Stephen Frost writes: > * Robert Haas (robertmh...@gmail.com) wrote: >> I'm really concerned about the security implications of this patch. I >> think we're setting ourselves up for a whole lot of hurt for somewhat >> unclear gain. > I'm certainly of a different opinion and, for the most part, I feel that > if there are security concerns then they need to be addressed- and > better by us than by asking users to use some other mechanism to > implement RLS. TBH, I found Robert's argument pretty persuasive. The idea that "SELECT * FROM table" might invoke arbitrary processing ought to scare anyone who's concerned about security, because that's going to completely break any assumptions about pg_dump being safe for instance, as well as force top-to-bottom rethinking of many other security assumptions. > ... That commit was > the ground-work to allow us to finally get proper RLS and I'm very > disappointed to hear that the mechanical pieces around making RLS easy > for users to use (and getting that check-box taken care of in a wide > variety of fields that we are being exposed to now, see the PGConf.NYC > keynote speakers...) is receiving such push-back. If this is being sold as merely "ease of use", then it is probably going to get rejected. In order to get some extra ease of use for the minority of users who need RLS, you are going to significantly complicate the lives of all Postgres users. That's not a net win in any sane calculation of ease of use. Maybe the right thing to think about is how we can make it easier to set up table + view combinations according to the pattern Robert described. I wouldn't have a problem with some more-or-less-automated support for doing that. (Consider SERIAL as a possible precedent here: it's basically a table creation macro.) > You're suggesting that we use views instead, which clearly could run > someone else's code. Perhaps the user will notice that they're > selecting from a view instead of a table, but I've never seen a security > design around making sure that what is being select'd from is a table > vs. a view. pg_dump is a sufficient counterexample to that statement. 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] Allowing NOT IN to use ANTI joins
On Wed, Jun 11, 2014 at 3:26 PM, Tom Lane wrote: > If we didn't have mechanisms like this, we'd have far worse hazards from > ALTER TABLE than whether the planner made an incorrect join optimization. > Consider ALTER COLUMN TYPE for instance. Obviously not general cases of ALTER COLUMN TYPE but dropping a NULL constraint seems like the kind of change targeted by Simon's "reduce lock strength" patch that I'm sure he's still interested in. I think that patch, while full of dragons to steer around, is something that will keep coming up again and again in the future. It's a huge operational risk that even these short exclusive locks can cause a huge production outage if they happen to get queued up behind a reporting query. I don't think it changes anything for this patch -- right now the world is arranged the way Tom described -- but it's something to keep in mind when we talk about lock strength reduction and the impact on existing queries. For example if there's an UPDATE query in repeatable read mode that has an IN clause like this and was optimized accordingly then any lock strength reduction patch would have to beware that an ALTER TABLE that dropped the NULL clause might impact the update query. Incidentally, Oracle has a feature for online schema changes that we might end up having to implement something similar. The good news is we have the infrastructure to maybe do it. The idea is to start capturing all the changes to the table using something like our logical changeset extraction. Then do the equivalent of "create newtable as select ... from oldtable" to create the new schema, then start replaying the accumulated changes to the new table. Eventually when the change queue drains then get an exclusive lock, drain any new changes, and swap in the new table with the new schema. -- greg -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] API change advice: Passing plan invalidation info from the rewriter into the planner?
* Robert Haas (robertmh...@gmail.com) wrote: > I'm really concerned about the security implications of this patch. I > think we're setting ourselves up for a whole lot of hurt for somewhat > unclear gain. I'm certainly of a different opinion and, for the most part, I feel that if there are security concerns then they need to be addressed- and better by us than by asking users to use some other mechanism to implement RLS. > In my view, commit 842faa714c0454d67e523f5a0b6df6500e9bc1a5 basically > *is* row-level security: instead of applying a row-level security > policy to a table, just create a security-barrier view over the table > and grant access to the view. Forget that the table ever existed. > Done. This argument could have been made for column-level privileges also, no? Yet I don't hear any calls for that to be ripped out now that you could implement it through updatable security-barrier views. That commit was the ground-work to allow us to finally get proper RLS and I'm very disappointed to hear that the mechanical pieces around making RLS easy for users to use (and getting that check-box taken care of in a wide variety of fields that we are being exposed to now, see the PGConf.NYC keynote speakers...) is receiving such push-back. > With this approach, there's a lot of stuff that we don't have to > reinvent. We've talked a lot about whether row-level security should > only be concerned with the rows it scans, or whether it should also > restrict the new rows that can be created. You can get either > behavior by choosing whether or not to use WITH CHECK OPTION. And > then there's this question of who should be RLS-exempt; that's > basically a question of to whom you grant privileges on the underlying > table. Note that this can be very fine-grained: for example, you can > allow someone to exempt themselves for selects but not for updates by > granting them SELECT privileges but not UPDATE privileges on the > underlying table. And potentially-exempt users can choose whether > they want a particular access to actually be exempt by targeting the > view when they don't want to be exempt and the table when they do. I agree that views, or even security-definer functions, offer a great deal of flexibility, and that may be necessary in some use-cases, but I fail to see why that means we should avoid providing the mechanics to achieve simple and usable RLS akin to what other major RDBMS's have. > That's mighty useful for debugging, at least IMHO. And, if you want > to have several row-level security policies for different classes of > users, just create more than one view and grant different privileges > on each. I'm really not impressed with the idea that RLS should be done with multiple different views of the same underlying table. > By contrast, it seems to me that every design so far proposed for > something that is actually called row-level security - as opposed to > commit 842faa714c0454d67e523f5a0b6df6500e9bc1a5, which *really is* > row-level security, is extremely limited. Look back at all the things > listed in the previous paragraph; can you do those things easily with > the designs that have been proposed? As far as I can see, not really. I don't feel that RLS will, or even *should*, have the same level of flexibility that you can achieve with views and/or security definer functions. I expect that, over time, we will add more capabilities to it, but it's never going to be able to redefine the contents of a column as a view can, nor will it be able to add columns to a table as views can. I don't see those as reasons against having support for RLS. > Your (Craig's) rls-9.4-upd-sb-views patch seems to have a rough > equivalent of WITH CHECK OPTION, probably because we've talked a lot > about that specific issue, but it doesn't line up exactly to what WITH > CHECK OPTION actually does. There's no independently-grantable > RLS-exemption privilege - and even when we talk about that, it's > usually some kind of global bit that applies to all tables and all > operations equally - whereas with the above approach it can be > per-table and per-operation and doesn't require superuser intervention > to flip the bit. I'm glad to hear your thoughts on the level of granularity which might be nice to have with RLS. What would be great is to spend a bit more time reviewing what other systems provide in this area and considering what makes sense for us. This will also be a feature and an area which we will be improving for a long time to come, but we do need this capability and we have to start somewhere. > There's no way for users who are RLS exempt to turn > off their exemption for testing purposes, let alone on a per-table > basis. I don't follow this argument entirely- users can't turn off the existing permissions system for testing either, unless an authorized user with the correct permissions makes the change to allow it- or the user bumps themselves up to superuser, or to a role
[HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
Hi all,
As part of GSoC2014 I'm sending a patch to add the capability of change an
unlogged table to logged [1].
I'll add it to the 9.5CF1.
Regards,
[1]
https://wiki.postgresql.org/wiki/Allow_an_unlogged_table_to_be_changed_to_logged_GSoC_2014
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 69a1e14..f822375 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -58,6 +58,7 @@ ALTER TABLE [ IF EXISTS ] name
ENABLE REPLICA RULE rewrite_rule_name
ENABLE ALWAYS RULE rewrite_rule_name
CLUSTER ON index_name
+SET LOGGED
SET WITHOUT CLUSTER
SET WITH OIDS
SET WITHOUT OIDS
@@ -432,6 +433,20 @@ ALTER TABLE [ IF EXISTS ] name
+SET LOGGED
+
+
+ This form change the table persistence type from unlogged to permanent by
+ rewriting the entire contents of the table and associated indexes into
+ new disk files.
+
+
+ Changing the table persistence type acquires an ACCESS EXCLUSIVE lock.
+
+
+
+
+
SET WITHOUT CLUSTER
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 341262b..f378f6a 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -136,7 +136,8 @@ static List *on_commits = NIL;
#define AT_PASS_ADD_INDEX 6 /* ADD indexes */
#define AT_PASS_ADD_CONSTR 7 /* ADD constraints, defaults */
#define AT_PASS_MISC 8 /* other stuff */
-#define AT_NUM_PASSES 9
+#define AT_PASS_SET_LOGGED 9 /* SET LOGGED */
+#define AT_NUM_PASSES 10
typedef struct AlteredTableInfo
{
@@ -376,6 +377,7 @@ static void ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMOD
static void ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId,
char *cmd, List **wqueue, LOCKMODE lockmode,
bool rewrite);
+static void ATPostAlterSetLogged(Oid relid);
static void TryReuseIndex(Oid oldId, IndexStmt *stmt);
static void TryReuseForeignKey(Oid oldId, Constraint *con);
static void change_owner_fix_column_acls(Oid relationOid,
@@ -402,6 +404,8 @@ static void ATExecAddOf(Relation rel, const TypeName *ofTypename, LOCKMODE lockm
static void ATExecDropOf(Relation rel, LOCKMODE lockmode);
static void ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKMODE lockmode);
static void ATExecGenericOptions(Relation rel, List *options);
+static void ATPrepSetLogged(Relation rel);
+static void ATExecSetLogged(Relation rel);
static void copy_relation_data(SMgrRelation rel, SMgrRelation dst,
ForkNumber forkNum, char relpersistence);
@@ -2855,6 +2859,7 @@ AlterTableGetLockLevel(List *cmds)
case AT_AddIndexConstraint:
case AT_ReplicaIdentity:
case AT_SetNotNull:
+ case AT_SetLogged:
cmd_lockmode = AccessExclusiveLock;
break;
@@ -3246,6 +3251,11 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
/* No command-specific prep needed */
pass = AT_PASS_MISC;
break;
+ case AT_SetLogged:
+ ATSimplePermissions(rel, ATT_TABLE); /* SET LOGGED */
+ ATPrepSetLogged(rel);
+ pass = AT_PASS_SET_LOGGED;
+ break;
default:/* oops */
elog(ERROR, "unrecognized alter table type: %d",
(int) cmd->subtype);
@@ -3308,6 +3318,9 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode)
ATPostAlterTypeCleanup(wqueue, tab, lockmode);
relation_close(rel, NoLock);
+
+ if (pass == AT_PASS_SET_LOGGED)
+ATPostAlterSetLogged(RelationGetRelid(rel));
}
}
@@ -3527,6 +3540,9 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
case AT_GenericOptions:
ATExecGenericOptions(rel, (List *) cmd->def);
break;
+ case AT_SetLogged:
+ ATExecSetLogged(rel);
+ break;
default:/* oops */
elog(ERROR, "unrecognized alter table type: %d",
(int) cmd->subtype);
@@ -10420,6 +10436,175 @@ ATExecGenericOptions(Relation rel, List *options)
}
/*
+ * ALTER TABLE SET LOGGED
+ *
+ * Change the table persistence type from unlogged to permanent by
+ * rewriting the entire contents of the table and associated indexes
+ * into new disk files.
+ *
+ * The ATPrepSetLogged function check all precondictions to perform
+ * the operation:
+ * - check if the target table is unlogged
+ * - check if not exists a foreign key to other unlogged table
+ */
+static void
+ATPrepSetLogged(Relation rel)
+{
+ Relation pg_constraint;
+ HeapTuple tuple;
+ SysScanDesc scan;
+ ScanKeyData skey[1];
+
+ /* check if is an unlogged relation */
+ if (rel->rd_rel->relpersistence != RELPERSISTENCE_UNLOGGED)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+ errmsg("table %s is not unlogged",
+ RelationGetRelatio
Re: [HACKERS] API change advice: Passing plan invalidation info from the rewriter into the planner?
* Craig Ringer (cr...@2ndquadrant.com) wrote: > On 06/11/2014 07:24 AM, Tom Lane wrote: > > Is the point of that that the table owner might have put trojan-horse > > functions into the RLS qual? If so, why are we only concerned about > > defending the superuser and not other users? Seems like the right fix > > would be to insist that functions in the RLS qual run as the table owner. > > Granted, that might be painful to do. But it still seems like "we only > > need to do this for superusers" is designing with blinkers on. > > I agree, and now that the urgency of trying to deliver this for 9.4 is > over it's worth seeing if we can just run as table owner. We'll need to work out how to ensure that things like current_user() still returns the calling user in that case, otherwise it won't make any sense. In general, I agree that having the RLS quals run as the table owner is a good approach and would love to hear suggestions about how we can make that happen. > Failing that, we could take the approach a certain other RDBMS does and > make the ability to define row security quals a GRANTable right > initially held only by the superuser. I don't particularly like this idea- it's akin, to me anyway, to making the ability to control other permissions on a table (SELECT, INSERT, etc) something which a user would have to be granted- and it doesn't really address the issue. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] API change advice: Passing plan invalidation info from the rewriter into the planner?
* Craig Ringer (cr...@2ndquadrant.com) wrote: > On 06/11/2014 02:19 AM, Tom Lane wrote: > > Hm ... I'm not following why we'd need a special case for superusers and > > not anyone else? Seems like any useful RLS scheme is going to require > > more privilege levels than just superuser and not-superuser. > > What it really needs is to invalidate plans when switching between > RLS-enabled and RLS-exempt users, yes. I'm sure we'll want an "RLS > exempt" right or mode sooner rather than later, so I'm against tying > this explicitly to superuser as such. That certainly sounds reasonable to me, but the point is we're just looking to see if the current role executing the plan should or should not have RLS applied and, if it's changing, we need to re-plan. We don't need to actually track an independent plan for each and every user executing the plan, which means that the plan cache can be largely left alone. > I wouldn't be surprised to see > > SET ROW SECURITY ON|OFF > > down the track, with a right controlling whether you can or not. Or at > least, a right that directly exempts a user from row security. Agreed, but doing a re-planning in that case seems reasonable to me. I find it pretty unlikely that there will be a lot of critical path cases of the same plan flipping back and forth between a role for which RLS is applied and a role where it shouldn't be. > > Could we put the "if superuser then ok" test into the RLS condition test > > and thereby not need more than one plan at all? > > Only if we put it in another level of security barrier subquery, because > otherwise the planner might execute the other quals (including possible > user defined functions) before the superuser test. Which was the whole > reason for the superuser test in the first place. Yeah, I'm not a big fan of this and it certainly seems a simpler approach to just force a re-plan. We're talking about a query which has been prepared and then is being executed by different roles, some of which are RLS enabled and some which are RLS exempt. That just strikes me as pretty unlikely to happen and if it does become an issue, a user could work around it by having two different plans prepared and making sure that they are called from the appropriate roles to avoid the replanning. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Proposing pg_hibernate
On Tue, Jun 10, 2014 at 10:03 PM, Gurjeet Singh wrote: > And it's probably accepted by now that such a bahviour is not > catastrophic, merely inconvenient. I think the whole argument for having pg_hibernator is that getting the block cache properly initialized is important. If it's not important, then we don't need pg_hibernator in the first place. But if it is important, then I think not loading unrelated blocks into shared_buffers is also important. -- 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] replication commands and log_statements
Andres Freund writes: > Your wish just seems like a separate feature to me. Including > replication commands in 'all' seems correct independent of the desire > for a more granular control. No, I think I've got to vote with the other side on that. The reason we can have log_statement as a scalar progression "none < ddl < mod < all" is that there's little visible use-case for logging DML but not DDL, nor for logging SELECTS but not INSERT/UPDATE/DELETE. However, logging replication commands seems like something people would reasonably want an orthogonal control for. There's no nice way to squeeze such a behavior into log_statement. I guess you could say that log_statement treats replication commands as if they were DDL, but is that really going to satisfy users? I think we should consider log_statement to control logging of SQL only, and invent a separate GUC (or, in the future, likely more than one GUC?) for logging of replication activity. 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] API change advice: Passing plan invalidation info from the rewriter into the planner?
On Tue, Jun 10, 2014 at 7:18 PM, Craig Ringer wrote: > On 06/11/2014 02:19 AM, Tom Lane wrote: >> Hm ... I'm not following why we'd need a special case for superusers and >> not anyone else? Seems like any useful RLS scheme is going to require >> more privilege levels than just superuser and not-superuser. > > What it really needs is to invalidate plans when switching between > RLS-enabled and RLS-exempt users, yes. I'm sure we'll want an "RLS > exempt" right or mode sooner rather than later, so I'm against tying > this explicitly to superuser as such. > > I wouldn't be surprised to see > > SET ROW SECURITY ON|OFF > > down the track, with a right controlling whether you can or not. Or at > least, a right that directly exempts a user from row security. I'm really concerned about the security implications of this patch. I think we're setting ourselves up for a whole lot of hurt for somewhat unclear gain. In my view, commit 842faa714c0454d67e523f5a0b6df6500e9bc1a5 basically *is* row-level security: instead of applying a row-level security policy to a table, just create a security-barrier view over the table and grant access to the view. Forget that the table ever existed. Done. With this approach, there's a lot of stuff that we don't have to reinvent. We've talked a lot about whether row-level security should only be concerned with the rows it scans, or whether it should also restrict the new rows that can be created. You can get either behavior by choosing whether or not to use WITH CHECK OPTION. And then there's this question of who should be RLS-exempt; that's basically a question of to whom you grant privileges on the underlying table. Note that this can be very fine-grained: for example, you can allow someone to exempt themselves for selects but not for updates by granting them SELECT privileges but not UPDATE privileges on the underlying table. And potentially-exempt users can choose whether they want a particular access to actually be exempt by targeting the view when they don't want to be exempt and the table when they do. That's mighty useful for debugging, at least IMHO. And, if you want to have several row-level security policies for different classes of users, just create more than one view and grant different privileges on each. By contrast, it seems to me that every design so far proposed for something that is actually called row-level security - as opposed to commit 842faa714c0454d67e523f5a0b6df6500e9bc1a5, which *really is* row-level security, is extremely limited. Look back at all the things listed in the previous paragraph; can you do those things easily with the designs that have been proposed? As far as I can see, not really. Your (Craig's) rls-9.4-upd-sb-views patch seems to have a rough equivalent of WITH CHECK OPTION, probably because we've talked a lot about that specific issue, but it doesn't line up exactly to what WITH CHECK OPTION actually does. There's no independently-grantable RLS-exemption privilege - and even when we talk about that, it's usually some kind of global bit that applies to all tables and all operations equally - whereas with the above approach it can be per-table and per-operation and doesn't require superuser intervention to flip the bit. There's no way for users who are RLS exempt to turn off their exemption for testing purposes, let alone on a per-table basis. There's no way to have multiple RLS policies on a single table. All of those are things that we get "for free" in the view-over-table model, and implementing formal RLS basically requires us to either invent a new RLS-specific way of doing each of those things, or suffer along with a subset of the functionality. Yuck. But what's really awful about this whole design is that it breaks the invariant that reading from a table doesn't run anybody else's code. It's already the case that users need to be awfully careful about modifying tables, because that might fire triggers that do bad things. But at least you can SELECT from a table and it will either work, or it will fail with a permission denied error. What it will not do is unexpectedly run some code that you weren't expecting it to run. You can't be so blithe about selecting from views, but reading a plain table is always OK. Now, as soon as we introduce the concept that selecting from a table might not really mean "read from the table" but "read from the table after applying this owner-specified qual", we're opening up a whole new set of attack surfaces. Every pg_dump is an opportunity to hack somebody else's account, or at least audit their activity. Protecting the superuser against everybody else is nice, but I think it's just as important to protect non-superusers against each other, and I think that's going to be hard -- because in the RLS world, SELECT * FROM tab is now *fundamentally* ambiguous. Maybe it's reading from the table, and maybe it's really clandestinely reading from a view over the table, and the user has no
Re: [HACKERS] Allowing NOT IN to use ANTI joins
Marti Raudsepp writes: > On Wed, Jun 11, 2014 at 11:53 AM, David Rowley wrote: >> as long as the transaction id >> is taken before we start planning in session 1 then it should not matter if >> another session drops the constraint and inserts a NULL value as we won't >> see that NULL value in our transaction... I'd assume that the transaction >> has to start before it grabs the table defs that are required for planning. >> Or have I got something wrong? > 1. You're assuming that query plans can only survive for the length of > a transaction. That's not true, prepared query plans can span many > transactions. > 2. Also a FOR UPDATE clause can return values "from the future", if > another transaction has modified the value and already committed. > But this whole issue is moot anyway, the plan will get invalidated > when the nullability changes. Right. The key point for David's concern is that we always hold (at least) AccessShareLock on every relation used in a query, continuously from rewrite through to the end of execution. This will block any attempt by other transactions to make schema changes in the relation(s). In the case of re-using a prepared plan, we re-acquire all these locks and then check to see if we received any invalidation messages that render the plan invalid; if not, we can proceed to execution with the same safety guarantees as originally. (If we did, we replan starting from the raw parse tree.) If we didn't have mechanisms like this, we'd have far worse hazards from ALTER TABLE than whether the planner made an incorrect join optimization. Consider ALTER COLUMN TYPE for instance. 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] [GENERAL] Question about partial functional indexes and the query planner
On Tue, Jun 10, 2014 at 7:19 PM, Tom Lane wrote: > Given the lack of previous complaints, I'm not sure this amounts to > a back-patchable bug, but it does seem like something worth fixing > going forward. Agreed, although I'd be willing to see us slip it into 9.4. It's doubtful that anyone will get upset if their query plans change between beta1 and beta2, but the same cannot be said for released branches. -- 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] Inaccuracy in VACUUM's tuple count estimates
Andres Freund wrote: > On 2014-06-09 11:24:22 -0700, Kevin Grittner wrote: >> The only way that it could be a problem is if the DELETE is in a >> subtransaction which might get rolled back without rolling back the >> INSERT. > > The way I understand the code in that case the subxid in xmax would have > been resolved the toplevel xid. > > /* > * Find top level xid. Bail out if xid is too early to be a conflict, or > * if it's our own xid. > */ > if (TransactionIdEquals(xid, GetTopTransactionIdIfAny())) > return; > xid = SubTransGetTopmostTransaction(xid); > if (TransactionIdPrecedes(xid, TransactionXmin)) > return; > if (TransactionIdEquals(xid, GetTopTransactionIdIfAny())) > return; > > That should essentially make that case harmless, right? Hmm. Since the switch statement above doesn't limit the HEAPTUPLE_DELETE_IN_PROGRESS or HEAPTUPLE_INSERT_IN_PROGRESS cases by visibility, we are safe. I had been thinking that we had early returns based on visibility, like the we do for HEAPTUPLE_LIVE and HEAPTUPLE_RECENTLY_DEAD. Since we don't do that, there is no problem with the code either before or after your recent change. Apologies that I didn't notice that sooner. It might be possible that with more guarantees of what those return codes mean (possibly by adding the new one mentioned by Tom) we could add that early return in one or both cases, which would better optimize some corner cases involving subtransactions. But now doesn't seem like a good time to worry about that. Such a micro-optimization would not be a sane candidate for back-patching, or for introducing this late in a release cycle. So, nothing to see here, folks. Move along. predicate.c is neutral in this matter. Good spot, BTW! -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_receivexlog add synchronous mode
On Tue, Jun 10, 2014 at 5:01 PM, wrote:
>> No. IIUC walreceiver does flush *less* frequently than what you
>> implemented on pg_receivexlog. Your version of pg_receivexlog tries to
>> do flush every time when it receives one WAL chunk. OTOH, walreceiver
>> does flush only when there is no extra WAL chunk in receive buffer. IOW,
>> after writing WAL chunk, if there is another WAL chunk that walreceiver
>> can receive immediately, it postpones flush later.
>>
>> > However, it seems difficult to apply as same way.
>>
>> Why? ISTM that's not so difficult.
>
> I was not able to understand movement of walreceiver well.
> While walreceiver writes data, do PQconsumeInput() by omitting the select().
> Do flush if the PQgetCopyData has been to return the zero continuously.
> Fixed to the same process using the flag.
You introduced the state machine using the flag "flush_flg" into pg_receivexlog.
That's complicated and would reduce the readability of the source code. I think
that the logic should be simpler like walreceiver's one.
Maybe I found one problematic path as follows:
1. WAL is written and flush_flag is set to 1
2. PQgetCopyData() returns 0 and flush_flg is incremented to 2
3. PQconsumeInput() is executed
4. PQgetCopyData() reads keepalive message
5. After processing keepalive message, PQgetCopyDate() returns 0
6. Since flush_flg is 2, WAL is flushed and flush_flg is reset to 0
But new message can arrive while processing keepalive message. Before
flushing WAL, such new message should be processed.
+Enables synchronous mode. If replication slot is disabled then
+this setting is irrelevant.
Why is that irrelevant in that case?
Even when replication slot is not used, thanks to this feature, pg_receivexlog
can flush WAL more proactively and which may improve the durability of WAL
which pg_receivexlog writes.
+printf(_(" -m, --sync-modesynchronous mode\n"));
I think that calling this feature "synchronous mode" is confusing.
Regards,
--
Fujii Masao
--
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] replication commands and log_statements
On 2014-06-11 14:50:34 +0200, Magnus Hagander wrote: > On Wed, Jun 11, 2014 at 2:35 PM, Andres Freund > wrote: > > > On 2014-06-11 14:22:43 +0200, Magnus Hagander wrote: > > > Yes, but how would you specify for example "i want DDL and all > > replication > > > commands" (which is quite a reasonable thing to log, I believe). If you > > > actually require it to be set to "all", most people won't have any use at > > > all for it... > > > > That's a reasonable feature request - but imo it's a separate > > one. It seems pretty noncontroversial and simple to make > > log_statement=all log replication commands. What you want - and you're > > sure not alone with that - is something considerably more complex... > > > > > I'm just saying if we're going to do it, let's do it right from the > beginning. Your wish just seems like a separate feature to me. Including replication commands in 'all' seems correct independent of the desire for a more granular control. > Or are you suggesting this would be something we'd backpatch? Thought about it for a sec, and then decided it doesn't seem warranted. 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] Inaccuracy in VACUUM's tuple count estimates
On 2014-06-09 11:24:22 -0700, Kevin Grittner wrote: > Andres Freund wrote: > > On 2014-06-09 09:45:12 -0700, Kevin Grittner wrote: > > > I am not sure, given predicate.c's coding, how > > HEAPTUPLE_DELETE_IN_PROGRESS could cause problems. Could you elaborate, > > since that's the contentious point with Tom? Since 'both in > > progress' > > can only happen if xmin and xmax are the same toplevel xid and you > > resolve subxids to toplevel xids I think it should currently be safe > > either way? > > The only way that it could be a problem is if the DELETE is in a > subtransaction which might get rolled back without rolling back the > INSERT. The way I understand the code in that case the subxid in xmax would have been resolved the toplevel xid. /* * Find top level xid. Bail out if xid is too early to be a conflict, or * if it's our own xid. */ if (TransactionIdEquals(xid, GetTopTransactionIdIfAny())) return; xid = SubTransGetTopmostTransaction(xid); if (TransactionIdPrecedes(xid, TransactionXmin)) return; if (TransactionIdEquals(xid, GetTopTransactionIdIfAny())) return; That should essentially make that case harmless, right? So it seems the optimization (and pessimization in other cases) of only tracking toplevel xids seems to save the day here? > If we ignore the conflict because we assume the INSERT > will be negated by the DELETE, and that doesn't happen, we would > get false negatives which would compromise correctness. If we > assume that the DELETE might not happen when the DELETE is not in a > separate subtransaction we might get a false positive, which would > only be a performance hit. If we know either is possible and have > a way to check in predicate.c, it's fine to check it there. Given the above I don't think this currently can happen. Am I understand it correctly? If so, it certainly deserves a comment... 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] replication commands and log_statements
On Wed, Jun 11, 2014 at 2:35 PM, Andres Freund wrote: > On 2014-06-11 14:22:43 +0200, Magnus Hagander wrote: > > Yes, but how would you specify for example "i want DDL and all > replication > > commands" (which is quite a reasonable thing to log, I believe). If you > > actually require it to be set to "all", most people won't have any use at > > all for it... > > That's a reasonable feature request - but imo it's a separate > one. It seems pretty noncontroversial and simple to make > log_statement=all log replication commands. What you want - and you're > sure not alone with that - is something considerably more complex... > > I'm just saying if we're going to do it, let's do it right from the beginning. Or are you suggesting this would be something we'd backpatch? Given the lack of complaints about it, I'd rather suggest we don't backpatch anything, and instead make the correct feature in the first pace for the next release. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] tests for client programs
Hi, On 2014-06-04 20:40:40 -0400, Peter Eisentraut wrote: > On Wed, 2014-05-07 at 03:08 +0200, Andres Freund wrote: > > > As an additional issue it currently doesn't seem to work in VPATH > > > builds. That's imo a must fix. > > > > A "cd $(srcdir) && .." in prove_installcheck and prove_check seems to do > > the trick. > > Here is my proposed patch for this. Works here. The tmpdir thing is probably a separate patch... 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] replication commands and log_statements
On 2014-06-11 14:22:43 +0200, Magnus Hagander wrote: > Yes, but how would you specify for example "i want DDL and all replication > commands" (which is quite a reasonable thing to log, I believe). If you > actually require it to be set to "all", most people won't have any use at > all for it... That's a reasonable feature request - but imo it's a separate one. It seems pretty noncontroversial and simple to make log_statement=all log replication commands. What you want - and you're sure not alone with that - is something considerably more complex... 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] replication commands and log_statements
On Wed, Jun 11, 2014 at 2:17 PM, Andres Freund wrote: > On 2014-06-11 13:42:58 +0200, Magnus Hagander wrote: > > On Wed, Jun 11, 2014 at 12:34 PM, Fujii Masao > wrote: > > > > > Hi, > > > > > > Replication commands like IDENTIFY_COMMAND are not logged even when > > > log_statements is set to all. Some users who use log_statements to > > > audit *all* statements might dislike this current situation. So I'm > > > thinking to change log_statements or add something like log_replication > > > so that we can log replication commands. Thought? > > > > > > > +1. I think adding a separate parameter is the way to go. > > Why? I can't really see a case where the log volume by replication > connections is going to be significant in comparison to 'all'? > > Yes, but how would you specify for example "i want DDL and all replication commands" (which is quite a reasonable thing to log, I believe). If you actually require it to be set to "all", most people won't have any use at all for it... -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] replication commands and log_statements
On 2014-06-11 13:42:58 +0200, Magnus Hagander wrote: > On Wed, Jun 11, 2014 at 12:34 PM, Fujii Masao wrote: > > > Hi, > > > > Replication commands like IDENTIFY_COMMAND are not logged even when > > log_statements is set to all. Some users who use log_statements to > > audit *all* statements might dislike this current situation. So I'm > > thinking to change log_statements or add something like log_replication > > so that we can log replication commands. Thought? > > > > +1. I think adding a separate parameter is the way to go. Why? I can't really see a case where the log volume by replication connections is going to be significant in comparison to 'all'? 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] replication commands and log_statements
On Wed, Jun 11, 2014 at 12:34 PM, Fujii Masao wrote: > Hi, > > Replication commands like IDENTIFY_COMMAND are not logged even when > log_statements is set to all. Some users who use log_statements to > audit *all* statements might dislike this current situation. So I'm > thinking to change log_statements or add something like log_replication > so that we can log replication commands. Thought? > +1. I think adding a separate parameter is the way to go. The other option would be to turn log_statements into a parameter that you specify multiple ones - so instead of "all" today it would be "ddl,dml,all" or something like that, and then you'd also add "replication" as an option. But that would cause all sorts of backwards compatibility annoyances.. And do you really want to be able to say things like "ddl,all" meanin you'd get ddl and select but not dml? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] replication commands and log_statements
On 2014-06-11 19:34:25 +0900, Fujii Masao wrote: > Hi, > > Replication commands like IDENTIFY_COMMAND are not logged even when > log_statements is set to all. Some users who use log_statements to > audit *all* statements might dislike this current situation. So I'm > thinking to change log_statements or add something like log_replication > so that we can log replication commands. Thought? Yes, I think we should do that. I can't really see it causing too much volume... 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] Compression of full-page-writes
On Wed, Jun 11, 2014 at 4:19 PM, Fujii Masao wrote: > > IIUC even when we adopt only one algorithm, additional at least one bit is > necessary to see whether this backup block is compressed or not. > > This flag is necessary only for backup block, so there is no need to use > the header of each WAL record. What about just using the backup block > header? > > +1. We can also steal a few bits from ForkNumber field in the backup block header if required. Thanks, Pavan -- Pavan Deolasee http://www.linkedin.com/in/pavandeolasee
Re: [HACKERS] Compression of full-page-writes
On Wed, Jun 11, 2014 at 10:05 AM, Michael Paquier wrote: > On Tue, Jun 10, 2014 at 11:49 PM, Rahila Syed wrote: >> Hello , >> >> >> In order to facilitate changing of compression algorithms and to be able to >> recover using WAL records compressed with different compression algorithms, >> information about compression algorithm can be stored in WAL record. >> >> XLOG record header has 2 to 4 padding bytes in order to align the WAL >> record. This space can be used for a new flag in order to store information >> about the compression algorithm used. Like the xl_info field of XlogRecord >> struct, 8 bits flag can be constructed with the lower 4 bits of the flag >> used to indicate which backup block is compressed out of 0,1,2,3. Higher >> four bits can be used to indicate state of compression i.e >> off,lz4,snappy,pglz. >> >> The flag can be extended to incorporate more compression algorithms added in >> future if any. >> >> What is your opinion on this? > -1 for any additional bytes in WAL record to control such things, > having one single compression that we know performs well and relying > on it makes the life of user and developer easier. IIUC even when we adopt only one algorithm, additional at least one bit is necessary to see whether this backup block is compressed or not. This flag is necessary only for backup block, so there is no need to use the header of each WAL record. What about just using the backup block header? Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] replication commands and log_statements
Hi, Replication commands like IDENTIFY_COMMAND are not logged even when log_statements is set to all. Some users who use log_statements to audit *all* statements might dislike this current situation. So I'm thinking to change log_statements or add something like log_replication so that we can log replication commands. Thought? Regards, -- Fujii Masao -- 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] Allowing NOT IN to use ANTI joins
On Wed, Jun 11, 2014 at 9:32 PM, Marti Raudsepp wrote: > On Sun, Jun 8, 2014 at 3:36 PM, David Rowley wrote: > > Currently pull_up_sublinks_qual_recurse only changes the plan for NOT > EXISTS > > queries and leaves NOT IN alone. The reason for this is because the > values > > returned by a subquery in the IN clause could have NULLs. > > There's a bug in targetListIsGuaranteedNotToHaveNulls, you have to > drill deeper into the query to guarantee the nullability of a result > column. If a table is OUTER JOINed, it can return NULLs even if the > original column specification has NOT NULL. > > This test case produces incorrect results with your patch: > > create table a (x int not null); > create table b (x int not null, y int not null); > insert into a values(1); > select * from a where x not in (select y from a left join b using (x)); > > Unpatched version correctly returns 0 rows since "y" will be NULL. > Your patch returns the value 1 from a. > > Thanks, I actually was just looking at that. I guess I'll just need to make sure that nothing in the targetlist comes from an outer join. Regards David Rowley
Re: [HACKERS] Allowing NOT IN to use ANTI joins
On Sun, Jun 8, 2014 at 3:36 PM, David Rowley wrote: > Currently pull_up_sublinks_qual_recurse only changes the plan for NOT EXISTS > queries and leaves NOT IN alone. The reason for this is because the values > returned by a subquery in the IN clause could have NULLs. There's a bug in targetListIsGuaranteedNotToHaveNulls, you have to drill deeper into the query to guarantee the nullability of a result column. If a table is OUTER JOINed, it can return NULLs even if the original column specification has NOT NULL. This test case produces incorrect results with your patch: create table a (x int not null); create table b (x int not null, y int not null); insert into a values(1); select * from a where x not in (select y from a left join b using (x)); Unpatched version correctly returns 0 rows since "y" will be NULL. Your patch returns the value 1 from a. Regards, Marti -- 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] Allowing NOT IN to use ANTI joins
On Wed, Jun 11, 2014 at 11:53 AM, David Rowley wrote: >> The only way to consistently guarantee nullability is through primary >> key constraints. Fortunately that addresses most of the use cases of >> NOT IN(), in my experience. >> See the comment in check_functional_grouping: > I saw that, but I have to say I've not fully got my head around why that's > needed just yet. I was wrong, see Tom's reply to my email. It's OK to rely on attnotnull for optimization decisions. The plan will be invalidated automatically when the nullability of a referenced column changes. check_functional_grouping needs special treatment because it decides whether to accept/reject views; and if it has allowed creating a view, it needs to guarantee that the dependent constraint isn't dropped for a longer term. > as long as the transaction id > is taken before we start planning in session 1 then it should not matter if > another session drops the constraint and inserts a NULL value as we won't > see that NULL value in our transaction... I'd assume that the transaction > has to start before it grabs the table defs that are required for planning. > Or have I got something wrong? 1. You're assuming that query plans can only survive for the length of a transaction. That's not true, prepared query plans can span many transactions. 2. Also a FOR UPDATE clause can return values "from the future", if another transaction has modified the value and already committed. But this whole issue is moot anyway, the plan will get invalidated when the nullability changes. Regards, Marti -- 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] Allowing NOT IN to use ANTI joins
On Tue, Jun 10, 2014 at 2:19 PM, Tom Lane wrote: > Jeff Janes writes: > > If you are using NOT IN, then once you find a NULL in the outer input (if > > the outer input is the in-list: clearly you can't reverse the two inputs > in > > this case), you don't even need to finish reading the outer input, nor > > start reading the inner input, because all rows are automatically > excluded > > by the weird semantics of NOT IN. > > The point I'm trying to make is that the goal of most join types is to > give an answer without having necessarily read all of either input. > For instance, if we tried to do this with a mergejoin it wouldn't work > reliably: it might suppose that it could accept an outer row on the basis > of no match in a higher-order sort column before it'd reached any nulls > appearing in lower-order sort columns. > > You might be right that we could hot-wire the hash join case in > particular, but I'm failing to see the point of expending lots of extra > effort in order to deliver a useless answer faster. If there are NULLs > in the inner input, then NOT IN is simply the wrong query to make, and > giving an empty output in a relatively short amount of time isn't going > to help clue the user in on that. (If the SQL standard would let us do > so, I'd be arguing for throwing an error if we found a NULL.) > > This got me thinking. It is probably a bit useless and wrong to perform a NOT IN when the subquery in the IN() clause can have NULL values, so I guess in any realistic useful case, the user will either have a NOT NULL constraint on the columns, or they'll do a WHERE col IS NOT NULL, so I should likely also allow a query such as: SELECT * FROM a WHERE id NOT IN(SELECT nullable_col FROM b WHERE nullable_col IS NOT NULL); to also perform an ANTI JOIN. I think it's just a matter of changing targetListIsGuaranteedNotToHaveNulls so that if it does not find the NOT NULL constraint, to check the WHERE clause of the query to see if there's any not null quals. I'm about to put this to the test, but if it works then I think it should cover many more cases for using NOT IN(), I guess we're only leaving out function calls and calculations in the target list. Regards David Rowley
Re: [HACKERS] Allowing NOT IN to use ANTI joins
On Mon, Jun 9, 2014 at 11:20 PM, Marti Raudsepp wrote: > On Sun, Jun 8, 2014 at 3:36 PM, David Rowley wrote: > > Currently pull_up_sublinks_qual_recurse only changes the plan for NOT > EXISTS > > queries and leaves NOT IN alone. The reason for this is because the > values > > returned by a subquery in the IN clause could have NULLs. > > I believe the reason why this hasn't been done yet, is that the plan > becomes invalid when another backend modifies the nullability of the > column. To get it to replan, you'd have to introduce a dependency on > the "NOT NULL" constraint, but it's impossible for now because there's > no pg_constraint entry for NOT NULLs. > > The only way to consistently guarantee nullability is through primary > key constraints. Fortunately that addresses most of the use cases of > NOT IN(), in my experience. > > I tried to break this by putting a break point in convert_ANY_sublink_to_join in session 1. Not that it really had to be in that function, I just wanted it to stop during planning and before the plan is executed. -- session 1 select * from n1 where id not in(select id from n1); -- hits breakpoint in convert_ANY_sublink_to_join -- session 2 alter table n2 alter column id drop not null; insert into n2 values(null); I see that session 2 blocks in the alter table until session 1 completes. I've not really checked out the code in detail around when the snapshot is taken and the transaction ID is generated, but as long as the transaction id is taken before we start planning in session 1 then it should not matter if another session drops the constraint and inserts a NULL value as we won't see that NULL value in our transaction... I'd assume that the transaction has to start before it grabs the table defs that are required for planning. Or have I got something wrong? > See the comment in check_functional_grouping: > > * Currently we only check to see if the rel has a primary key that is a > * subset of the grouping_columns. We could also use plain unique > constraints > * if all their columns are known not null, but there's a problem: we need > * to be able to represent the not-null-ness as part of the constraints > added > * to *constraintDeps. FIXME whenever not-null constraints get represented > * in pg_constraint. > > I saw that, but I have to say I've not fully got my head around why that's needed just yet. > The behavior you want seems somewhat similar to > check_functional_grouping; maybe you could unify it with your > targetListIsGuaranteedNotToHaveNulls at some level. (PS: that's one > ugly function name :) > > Agreed :) Originally I had put the code that does that in convert_ANY_sublink_to_join, but at the last minute before posting the patch I decided that it might be useful and reusable so moved it out to that function. I'll try and think of something better, but I'm open to ideas. Regards David Rowley
