[HACKERS] Bug? Concurrent COMMENT ON and DROP object
In the following scenario, we can see orphan comments.
session.1 session.2
----
1: CREATE TYPE my_typ
AS (a int, b text);
2: BEGIN;
3: COMMENT ON TYPE my_typ
IS 'testtest';
4: DROP TYPE my_typ;
5: COMMIT;
SELECT * FROM pg_description
WHERE description = 'testtest';
objoid | classoid | objsubid | description
+--+--+-
16393 | 1247 |0 | testtest
(1 row)
----
The CommentRelation() has the following code:
| static void
| CommentRelation(int objtype, List *relname, char *comment)
| {
| Relationrelation;
| RangeVar *tgtrel;
|
| tgtrel = makeRangeVarFromNameList(relname);
|
| /*
| * Open the relation. We do this mainly to acquire a lock that ensures no
| * one else drops the relation before we commit. (If they did, they'd
| * fail to remove the entry we are about to make in pg_description.)
| */
| relation = relation_openrv(tgtrel, AccessShareLock);
|:
|:
| /* Done, but hold lock until commit */
| relation_close(relation, NoLock);
| }
It says the purpose of the relation_openrv() to acquire a lock that
ensures no one else drops the relation before we commit. So, I was
blocked when I tried to comment on the table which was already dropped
in another session but uncommited yet.
However, it is not a problem limited to relations. For example, we need
to acquire a lock on the pg_type catalog using
For example, we need to acquire a lock on the pg_type catalog when we
try to comment on any type object. Perhaps, I think LockRelationOid()
should be injected at head of the CommentType() in this case.
Any comments?
--
KaiGai Kohei kai...@ak.jp.nec.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: I: [HACKERS] About Our CLUSTER implementation is pessimal patch
Leonardo F m_li...@yahoo.it wrote: Attached the updated patch (should solve a bug) and a script. I reviewed your patch. It seems to be in good shape, and worked as expected. I suppressed a compiler warning in the patch and cleaned up whitespaces in it. Patch attached. I think we need some documentation for the change. The only downside of the feature is that sorted cluster requires twice disk spaces of the target table (temp space for disk sort and the result table). Could I ask you to write documentation about the new behavior? Also, code comments can be improved; especially we need better description than copypaste from FormIndexDatum. Regards, --- Takahiro Itagaki NTT Open Source Software Center sorted_cluster-20100706.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: I: [HACKERS] About Our CLUSTER implementation is pessimal patch
I reviewed your patch. It seems to be in good shape, and worked as expected. I suppressed a compiler warning in the patch and cleaned up whitespaces in it. Patch attached. Thanks for the review! I saw that you also changed the writing: LogicalTapeWrite(state-tapeset, tapenum, tuple, HEAPTUPLESIZE); LogicalTapeWrite(state-tapeset, tapenum, tuple-t_data, tuple-t_len-HEAPTUPLESIZE); and the reading: tuple-t_len = tuplen - HEAPTUPLESIZE; if (LogicalTapeRead(state-tapeset, tapenum, tuple-t_self, tuplen-sizeof(tuplen)) != tuplen-sizeof(tuplen)) elog(ERROR, unexpected end of data); into (writing): LogicalTapeWrite(state-tapeset, tapenum, tuple, tuple-t_len); (reading): if (LogicalTapeRead(state-tapeset, tapenum, tuple-t_self, tuplen-sizeof(tuplen)) != tuplen-sizeof(tuplen)) Are we sure it's 100% equivalent? I remember I had issues with the fact that tuple-t_data wasn't at HEAPTUPLESIZE distance from tuple: http://osdir.com/ml/pgsql-hackers/2010-02/msg00744.html I think we need some documentation for the change. The only downside of the feature is that sorted cluster requires twice disk spaces of the target table (temp space for disk sort and the result table). Could I ask you to write documentation about the new behavior? Also, code comments can be improved; especially we need better description than copypaste from FormIndexDatum. I'll try to improve the comments and add doc changes (but my English will have to be double checked...) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug? Concurrent COMMENT ON and DROP object
2010/7/6 KaiGai Kohei kai...@ak.jp.nec.com: In the following scenario, we can see orphan comments. Yeah. I think the reason we haven't seen any complaints about this before is that the worst-case scenario is that a comment for a dropped database object eventually becomes associated with a new database object. But that can only happen if the OID counter wraps around and then OID then gets reused for another object of the same type, and even then you might easily fail to notice. Still, it would be nice to clean this up. It says the purpose of the relation_openrv() to acquire a lock that ensures no one else drops the relation before we commit. So, I was blocked when I tried to comment on the table which was already dropped in another session but uncommited yet. However, it is not a problem limited to relations. For example, we need to acquire a lock on the pg_type catalog using For example, we need to acquire a lock on the pg_type catalog when we try to comment on any type object. Perhaps, I think LockRelationOid() should be injected at head of the CommentType() in this case. Any comments? A more fine-grained lock would be preferable, if we can manage it. Can we just lock the relevant pg_type tuple, rather than the whole relation? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres 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] t_self as system column
On Mon, Jul 5, 2010 at 3:26 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Mon, Jul 5, 2010 at 2:08 PM, Tom Lane t...@sss.pgh.pa.us wrote: At one time I was hoping to get rid of explicit entries in pg_attribute for system columns, which would negate this concern. I think we're stuck with them now, though, because of per-column permissions. Because someone might want to grant per-column permissions on those columns? That seems like an awfully thin reason to keep all that bloat around. I bet the number of people who have granted per-column permissions on, say, cmax can be counted on one hand - possibly with five fingers left over. I'd agree with that argument for the most part, but I'm not entirely sure about oid, which has some characteristics of a user-data column. (OTOH, maybe we could allow just oid to retain an explicit pg_attribute entry... could be messy though.) [woops, forgot to reply on-list] Treating OID as a user-defined column seems reasonable, and probably not even that messy if we put some appropriate macros in place. I'm guessing the messy part would be finding all the places that expect to be consulting a real pg_attribute row and supplying them with a faked-up one in its place. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres 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] Re: [COMMITTERS] pgsql: Fix log_temp_files docs and comments to say bytes not kilobytes.
On Fri, Jun 25, 2010 at 1:56 PM, Josh Berkus j...@agliodbs.com wrote: Shouldn't this be backpatched, or was this a new bug in 9.0? We've always output bytes. I'd have noticed the discrepancy myself if I'd read the actual docs ;-) KB would be more useful. And I don't think people have enough scripts built on this yet to make this break anything. We should backport to 8.4. log_temp_files was introduced in 8.3, so we'll need to backpatch this to 8.3, not just 8.4. Greg Smith tells me Simon has been busy with other things, so I'm going to pick this up. Barring objections, I'm going to revert Simon's patch and change the behavior instead, per the attached patch. I will also back-patch the fix to 8.4 and 8.3. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company log_temp_files.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug? Concurrent COMMENT ON and DROP object
Robert Haas robertmh...@gmail.com writes: 2010/7/6 KaiGai Kohei kai...@ak.jp.nec.com: In the following scenario, we can see orphan comments. Yeah. I think the reason we haven't seen any complaints about this before is that the worst-case scenario is that a comment for a dropped database object eventually becomes associated with a new database object. Well, in general there is very little DDL locking for any object type other than tables. I think the original rationale for that was that most other object types are defined by single catalog entries, so that attempts to update/delete the object would naturally block on changing its tuple anyway. But between comments and pg_depend entries that seems not particularly true anymore. IIRC there is now some attempt to lock objects of all types during DROP. Maybe the COMMENT code could acquire a conflicting lock. For example, we need to acquire a lock on the pg_type catalog when we try to comment on any type object. Perhaps, I think LockRelationOid() should be injected at head of the CommentType() in this case. Any comments? A more fine-grained lock would be preferable, s/preferable/essential/. This cure would be *far* worse than the disease. Can you say deadlock? 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] Re: [COMMITTERS] pgsql: Fix log_temp_files docs and comments to say bytes not kilobytes.
Robert Haas robertmh...@gmail.com writes: log_temp_files was introduced in 8.3, so we'll need to backpatch this to 8.3, not just 8.4. Greg Smith tells me Simon has been busy with other things, so I'm going to pick this up. Barring objections, I'm going to revert Simon's patch and change the behavior instead, per the attached patch. I will also back-patch the fix to 8.4 and 8.3. I agree with doing this for 9.0, but I'm much less sold that it's a good idea to change the meaning of the parameter in minor releases of 8.4 and 8.3. Aren't you risking breaking installations that work now? 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] Re: [COMMITTERS] pgsql: Fix log_temp_files docs and comments to say bytes not kilobytes.
Robert Haas wrote: On Fri, Jun 25, 2010 at 1:56 PM, Josh Berkus j...@agliodbs.com wrote: Shouldn't this be backpatched, or was this a new bug in 9.0? We've always output bytes. I'd have noticed the discrepancy myself if I'd read the actual docs ;-) KB would be more useful. And I don't think people have enough scripts built on this yet to make this break anything. We should backport to 8.4. log_temp_files was introduced in 8.3, so we'll need to backpatch this to 8.3, not just 8.4. Greg Smith tells me Simon has been busy with other things, so I'm going to pick this up. Barring objections, I'm going to revert Simon's patch and change the behavior instead, per the attached patch. I will also back-patch the fix to 8.4 and 8.3. I was expecting Josh's suggestion to get argued down...I'm curious why anyone is considering a behavior change in 8.3 or 8.4 over this? It's reasonable to change the behavior in 9.0, and/or to correct the docs for 8.3 or 8.4, but I would think a behavior change in the earlier versions would be out of line with the project's version policies. I know I have scripts that will mysteriously break if this goes into a stable version. I'm used to writing things that have code for detecting major version and acting accordingly, but I'd really prefer this project doesn't ever wander down the path where I need to start writing in code for do this on =8.3.11, this other thing for =8.3.12. -- Greg Smith 2ndQuadrant US Baltimore, MD PostgreSQL Training, Services and Support g...@2ndquadrant.com www.2ndQuadrant.us -- 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] Re: [COMMITTERS] pgsql: Fix log_temp_files docs and comments to say bytes not kilobytes.
On Tue, Jul 6, 2010 at 10:42 AM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: log_temp_files was introduced in 8.3, so we'll need to backpatch this to 8.3, not just 8.4. Greg Smith tells me Simon has been busy with other things, so I'm going to pick this up. Barring objections, I'm going to revert Simon's patch and change the behavior instead, per the attached patch. I will also back-patch the fix to 8.4 and 8.3. I agree with doing this for 9.0, but I'm much less sold that it's a good idea to change the meaning of the parameter in minor releases of 8.4 and 8.3. Aren't you risking breaking installations that work now? I think my least favorite option is changing the behavior only in HEAD. I think the reasonable options are: 1. Change the behavior in HEAD, 8.4, and 8.3, per previous discussion. If we do this, we should do what I proposed in my previous email. 2. Change the comments and documentation in 8.4 and 8.3 along the lines that Simon already did in HEAD. If we do this, we also need to change the GUC units to something other than GUC_UNIT_KB, as noted upthread. I'm not sure what would be appropriate. The reason I think it's OK to change the behavior in the back-branches is that (a) the only thing it affects is logging, so it shouldn't really break anything, and (b) apparently nobody has noticed that the interpretation of the GUC is off by three orders of magnitude, so either nobody's using it or they're not looking at what's actually happening too carefully. But I'm OK with going the other way and changing the code and docs in the back-branches, too. I just think we should be consistent. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres 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] 64-bit pgbench V2
Robert Haas wrote: It doesn't seem very palatable to have multiple handwritten integer parsers floating around the code base either. Maybe we should try to standardize something and ship it in src/port, or somesuch I was considering at one point making two trips through strtol, each allowed to gobble 10 characters, then combining the two--just to cut down a little bit on the roll your own parser aspects here. I hadn't really considered how the main server does this job though. If there's something reasonable to expose by refactoring some code that's already there, I could take a stab at that. I'm not exactly sure where the integer parsing code in the server that would be appropriate is to break out is at though. -- Greg Smith 2ndQuadrant US Baltimore, MD PostgreSQL Training, Services and Support g...@2ndquadrant.com www.2ndQuadrant.us -- 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] 64-bit pgbench V2
On Tue, Jul 6, 2010 at 11:01 AM, Greg Smith g...@2ndquadrant.com wrote: Robert Haas wrote: It doesn't seem very palatable to have multiple handwritten integer parsers floating around the code base either. Maybe we should try to standardize something and ship it in src/port, or somesuch I was considering at one point making two trips through strtol, each allowed to gobble 10 characters, then combining the two--just to cut down a little bit on the roll your own parser aspects here. I hadn't really considered how the main server does this job though. If there's something reasonable to expose by refactoring some code that's already there, I could take a stab at that. I'm not exactly sure where the integer parsing code in the server that would be appropriate is to break out is at though. Take a look at int8in. It's got some backend-specific stuff in it ATM but maybe it would be reasonable to try to fact that out somehow. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres 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] Re: [COMMITTERS] pgsql: Fix log_temp_files docs and comments to say bytes not kilobytes.
Robert Haas wrote: I think my least favorite option is changing the behavior only in HEAD. I think the reasonable options are: 1. Change the behavior in HEAD, 8.4, and 8.3, per previous discussion. If we do this, we should do what I proposed in my previous email. 2. Change the comments and documentation in 8.4 and 8.3 along the lines that Simon already did in HEAD. If we do this, we also need to change the GUC units to something other than GUC_UNIT_KB, as noted upthread. I'm not sure what would be appropriate. The reason I think it's OK to change the behavior in the back-branches is that (a) the only thing it affects is logging, so it shouldn't really break anything, and (b) apparently nobody has noticed that the interpretation of the GUC is off by three orders of magnitude, so either nobody's using it or they're not looking at what's actually happening too carefully. But I'm OK with going the other way and changing the code and docs in the back-branches, too. I just think we should be consistent. I normally don't backpatch anything unless it is either a possible cause of data loss, or a problem that is reported by multiple people. Anything backpatched risks causing instability, and might discourage people from performing minor upgrades. Minor fixes are rarely worth the risk of causing instability in back-branches. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + None of us is going to be here forever. + -- 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] Re: [COMMITTERS] pgsql: Fix log_temp_files docs and comments to say bytes not kilobytes.
On Tue, Jul 6, 2010 at 11:03 AM, Bruce Momjian br...@momjian.us wrote: Robert Haas wrote: I think my least favorite option is changing the behavior only in HEAD. I think the reasonable options are: 1. Change the behavior in HEAD, 8.4, and 8.3, per previous discussion. If we do this, we should do what I proposed in my previous email. 2. Change the comments and documentation in 8.4 and 8.3 along the lines that Simon already did in HEAD. If we do this, we also need to change the GUC units to something other than GUC_UNIT_KB, as noted upthread. I'm not sure what would be appropriate. The reason I think it's OK to change the behavior in the back-branches is that (a) the only thing it affects is logging, so it shouldn't really break anything, and (b) apparently nobody has noticed that the interpretation of the GUC is off by three orders of magnitude, so either nobody's using it or they're not looking at what's actually happening too carefully. But I'm OK with going the other way and changing the code and docs in the back-branches, too. I just think we should be consistent. I normally don't backpatch anything unless it is either a possible cause of data loss, or a problem that is reported by multiple people. Anything backpatched risks causing instability, and might discourage people from performing minor upgrades. Minor fixes are rarely worth the risk of causing instability in back-branches. OK. Well, in that case, I think we should backpatch the changes Simon already made, and also pick a new unit for the GUC. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres 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] Re: [COMMITTERS] pgsql: Fix log_temp_files docs and comments to say bytes not kilobytes.
Robert Haas robertmh...@gmail.com writes: The reason I think it's OK to change the behavior in the back-branches is that (a) the only thing it affects is logging, so it shouldn't really break anything, and (b) apparently nobody has noticed that the interpretation of the GUC is off by three orders of magnitude, so either nobody's using it or they're not looking at what's actually happening too carefully. It might be that nobody's using any values other than 0 and -1 ... in which case it wouldn't matter anyway. I agree that the lack of bug reports is notable. But still, don't we try to avoid behavioral changes in stable branches? I'm not dead set against doing what you propose, just think it needs some discussion. 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] Re: [COMMITTERS] pgsql: Fix log_temp_files docs and comments to say bytes not kilobytes.
Robert Haas robertmh...@gmail.com writes: On Tue, Jul 6, 2010 at 11:03 AM, Bruce Momjian br...@momjian.us wrote: Anything backpatched risks causing instability, and might discourage people from performing minor upgrades. Minor fixes are rarely worth the risk of causing instability in back-branches. OK. Well, in that case, I think we should backpatch the changes Simon already made, and also pick a new unit for the GUC. Changing the unit setting would also be a behavioral change. I think what Bruce is suggesting is that this is simply not worth worrying about in the back branches. 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] Re: [COMMITTERS] pgsql: Fix log_temp_files docs and comments to say bytes not kilobytes.
On Tue, Jul 6, 2010 at 11:10 AM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Tue, Jul 6, 2010 at 11:03 AM, Bruce Momjian br...@momjian.us wrote: Anything backpatched risks causing instability, and might discourage people from performing minor upgrades. Minor fixes are rarely worth the risk of causing instability in back-branches. OK. Well, in that case, I think we should backpatch the changes Simon already made, and also pick a new unit for the GUC. Changing the unit setting would also be a behavioral change. I think what Bruce is suggesting is that this is simply not worth worrying about in the back branches. It seems pretty strange not to at least document it. And I'm not wild about adding documentation that says Even though this value purports to be in kilobytes, it's really not, but I guess we can. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres 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] Re: [COMMITTERS] pgsql: Fix log_temp_files docs and comments to say bytes not kilobytes.
Robert Haas robertmh...@gmail.com writes: On Tue, Jul 6, 2010 at 11:10 AM, Tom Lane t...@sss.pgh.pa.us wrote: Changing the unit setting would also be a behavioral change. I think what Bruce is suggesting is that this is simply not worth worrying about in the back branches. It seems pretty strange not to at least document it. And I'm not wild about adding documentation that says Even though this value purports to be in kilobytes, it's really not, but I guess we can. Uh, no, the suggestion is to do *nothing* in the back branches. Yes they're buggy, but without any field complaints, it's hard to argue that anyone much cares. And I agree with Greg Smith that for anyone who does care, a behavioral change in a minor release is much harder to deal with than a change at a major release. 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] Re: [COMMITTERS] pgsql: Fix log_temp_files docs and comments to say bytes not kilobytes.
On Tue, Jul 6, 2010 at 11:25 AM, Tom Lane t...@sss.pgh.pa.us wrote: without any field complaints, I refer you to Simon's original commit message: Bug found during recent performance tuning for PostgreSQL user. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres 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] TRUNCATE+COPY optimization and --jobs=1 in pg_restore
2010/2/10 Takahiro Itagaki itagaki.takah...@oss.ntt.co.jp Tom Lane t...@sss.pgh.pa.us wrote: Takahiro Itagaki itagaki.takah...@oss.ntt.co.jp writes: We have an optimization to bulkload date in pg_restore, but the code only works in parallel restore (--jobs = 2). Why don't we do the same optimization in the serial restore (--jobs = 1) ? The code is only trying to substitute for something you can't have in parallel restore, ie --single-transaction. Yeah, the comment says so. But it does not necessarily mean that we cannot optimize the copy also in non-single-transaction restore. The attached patch improve the judgment condition, I'll add it to the next commit-fest. Hi This is my first review, so I hope I won't do too many things wrong. Please tell me how to improve. I've taken all the points from the 'reviewing a patch' wiki page. But to sum up what is below, I did see a large performance improvement (in a very simple test case) and no problems with the patch. Submission review Is the patch in context diff format? Yes Does it apply cleanly to the current CVS HEAD? Yes Does it include reasonable tests, necessary doc patches, etc? Not applicable: two lines updated Usability review Not applicable, doesn't change the use of pg_restore Read what the patch is supposed to do, and consider: Does the patch actually implement that? Yes: it wraps the truncate table + copy in a single transaction when doing pg_restore -c Do we want that? I think so, see the improvements below. And it makes the performance consistent between -j, -1 and 'simple' uses of pg_restore. Do we already have it? No Does it follow SQL spec, or the community-agreed behavior? Yes: if this is the way to do restore with -j, there is no point in not doing it with simple restores Does it include pg_dump support (if applicable)? Not applicable Are there dangers? I dont think so. The patch replaces the (is_parallel te-created) with (!ropt-single_txn te-created) to wrap every copy during restore in a transaction, preceding it with a truncate. This can only be done if the restore isn't already 'single transaction' (and there is no point in doing it in that case), and if we successfully created the table in our restore work, so the table is empty. The purpose being not doing unnecessary wal logging if possible. Feature test Apply the patch, compile it and test: Does the feature work as advertised? Yes. COPY is preceded by BEGIN and TRUNCATE when doing restore, and followed by COMMIT. This happens if and only if the table has been created during the restore. If the table was already there and restore appends data in it, only COPY is run. This was checked when explicitely restoring only data (-a, no truncate), and when restoring structure AND data (truncate only if creating is really done, not in the case of error because the table was already there). No WAL was generated. Are there corner cases the author has failed to consider? I don't think so Are there any assertion failures or crashes? Not during my tests Performance review Does the patch slow down simple tests? No If it claims to improve performance, does it? Yes. Test case : one 10 million rows table, single int column, no indexes. One single SATA 5400rpm disk, laptop. Dump was generated with pg_dump -Fc -Z0. A checkpoint was triggered between each run. wal_sync_method to fdatasync. First test (default configuration), wal_buffers=64kB, checkpoint_segments=3, shared_buffers=32MB. With patch, restore in 15s, without, 38s. Second test, wal_buffers=512kB, checkpoint_segments=32, shared_buffers=512MB. With patch, restore in 15s, without, 36s (this is a laptop, it is IO-bound during this test). Does it slow down other things? It didn't seem to, and there is no reason why it should. Coding review Read the changes to the code in detail and consider: Does it follow the project coding guidelines? Yes, to my knowledge Are there portability issues? No Will it work on Windows/BSD etc? Yes Are the comments sufficient and accurate? Yes, comments were modified to explain the new behaviour Does it do what it says, correctly? Yes Does it produce compiler warnings? No Can you make it crash? No Architecture review Consider the changes to the code in the context of the project as a whole: Is everything done in a way that fits together coherently with other features/modules? Yes, it's a 2 line-patch for the code, and 5 lnes of doc. Are there interdependencies that can cause problems? No Again, this is my first. Please tell me how to improve.
[HACKERS] keepalive in libpq using
Hello all. While I'm very excited about enabling keepalives in libpq, I want to know how can I use this functionality in my application? Let's imagine that I connect to a server with keepalives option, other options (keepalives_idle, keepalives_interval, keepalives_count) are used either. Then network goes down. So, how will I know that connection is dead? Any callback function? Or should I check PQstatus periodically? Thank in advance -- Nullus est in vitae sensus ipsa vera est sensus. -- 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] Partitioning syntax
On Thu, Jun 17, 2010 at 9:34 PM, Takahiro Itagaki itagaki.takah...@oss.ntt.co.jp wrote: Jaime Casanova ja...@2ndquadrant.com wrote: This one, doesn't apply to head anymore... please update Thank you for reviewing my patch! I attached an updated patch set for partitioning syntax. I've taken a little bit more of a look at this patch and I guess I'm not too happy with the design. 1. It seems to me that the proposed design for pg_partition is poorly thought out. In particular, I don't see how this would work if we wanted to partition on multiple keys, which is a feature supported by both Oracle and MySQL. It would also be nice to give at least some thought to how we might handle partitioning by list with subpartitioning by range or hash, or range partitioning with subpartitioning by hash. We certainly don't need to do subpartitioning in the first version of the patch, but I think we should have a plan. 2. I am still of the view that the first version of this patch should correctly handle routing of INSERT and COPY data to the correct partition. But at a very minimum we need to have a plan for how we're going to implement that in a follow-on patch. I think the way to do this is to binary search a sorted array of partition keys (perhaps upper bounds for range partitioning, and exact values for list partitioning). When you find the correct key, then you find the index of that key and look up that same index in a separate array of table OIDs and insert there. While it's possible to construct such a structure from the proposed catalog structure, it requires an index scan. I'm wondering if it might be better to abandon the idea of storing the partition values in pg_inherits and instead put preconstructed arrays directly into pg_partition. That way, with a single row fetch, you can get all the data you need. I'm not sure this is better, though - other opinions? 3. For a first version of this patch, I would suggest that we only allow partitioning by base columns, rather than expressions. When someone goes to do a bulk load of data into the table, and we want to do automatic tuple routing, we're going to have to evaluate the partitioning expression(s) for every row. I'm just guessing here, but I bet it's a lot cheaper to fetch an attribute by attnum than to evaluate an arbitrary expression. So even if we add partitioning by expression later, I don't think that the work to make a special case for base columns will be wasted. 4. The dependency handling needs more thought. For example, if I do this: create table names (id integer primary key, name varchar not null) partition by range (id) (partition names_1 values less than 1000, partition names_2 values less than 2000, partition names_3 values less than 3000); and then I drop names_2, the automatically generated constraint on names_3 still says CHECK (2000 = id AND id 3000). And I can drop those constraints off the individual tables, too, which doesn't seem like it ought to be allowed. And if I do something like this: create or replace function f(int) returns int as $$select $1$$ immutable language sql; create table names (id integer primary key, name varchar not null) partition by range (f(id)) (partition names_1 values less than 1000, partition names_2 values less than 2000, partition names_3 values less than 3000); alter table names_1 drop constraint names_1_id_check; alter table names_2 drop constraint names_2_id_check; alter table names_3 drop constraint names_3_id_check; drop function f(int); ...then I get: ERROR: cannot drop function f(integer) because other objects depend on it DETAIL: range partition depends on function f(integer) ...but there's no identification of which range partition depends on it. The locking is also broken here. In session #1, start a transaction and do CREATE PARTITION on an existing partitioned table. Then in session #2, do DROP FUNCTION some function CASCADE in a manner that leads to the range partition getting dropped. Then commit session #1. Now you have a pg_inherits catalog with leftovers in inhvalues. 5. The use of the term partition is not very consistent. For example, we use CREATE PARTITION to create a partition, but we use DROP TABLE to get rid of it (there is no DROP PARTITION). I think that the right syntax to use here is ALTER TABLE ... ADD/DROP PARTITION; both Oracle and MySQL do it that way. And meanwhile OCLASS_PARTITION means the partitioning information associated with the parent table, not a partition of a parent table. 6. There's some kind of magic in here associated with indexes on the parent table - it seems that matching indexes or primary keys are automatically created on each child table. But there's no provision for keeping them in sync. If I create a partitioned table with a primary key, the key is inherited by all its current children. If I then drop the primary key, it disappears from the parent but it still exists on the children. Any new children created
Re: [HACKERS] Re: [COMMITTERS] pgsql: Fix log_temp_files docs and comments to say bytes not kilobytes.
On Tue, Jul 6, 2010 at 11:25 AM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Tue, Jul 6, 2010 at 11:10 AM, Tom Lane t...@sss.pgh.pa.us wrote: Changing the unit setting would also be a behavioral change. I think what Bruce is suggesting is that this is simply not worth worrying about in the back branches. It seems pretty strange not to at least document it. And I'm not wild about adding documentation that says Even though this value purports to be in kilobytes, it's really not, but I guess we can. Uh, no, the suggestion is to do *nothing* in the back branches. Yes they're buggy, but without any field complaints, it's hard to argue that anyone much cares. And I agree with Greg Smith that for anyone who does care, a behavioral change in a minor release is much harder to deal with than a change at a major release. OK, so I talked to Bruce about this and I guess I've been persuaded that we should just apply the patch I sent upthread to HEAD and leave the back-branches broken, for fear of creating an incompatibility. I'll go do that unless someone wants to argue further... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres 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] logistics for beta3
On mån, 2010-07-05 at 11:01 -0400, Tom Lane wrote: Peter was suggesting that if we *stopped* using RTLD_GLOBAL then it might be possible to use plpython2 and plpython3 concurrently in one backend. After looking at the archives I'm not convinced that's workable --- it sounds like not using RTLD_GLOBAL would have the effect of breaking Python's extension scheme altogether. Yeah, plpython regression tests fail of you change RTLD_GLOBAL to RTLD_LOCAL. I will make a note in the documentation that using plpython2 and 3 together doesn't work. -- 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] logistics for beta3
Robert Haas wrote: On Mon, Jul 5, 2010 at 3:14 PM, Andrew Dunstan and...@dunslane.net wrote: Andrew Dunstan wrote: Marc G. Fournier wrote: - Someone (presumably Bruce) needs to run pgindent. ?Any reason to wait any longer on that? The typedefs list on the buildfarm needs to be refreshed. That will take me some time, since I wasn't aware we were about to do a pg_indent run. Starting now ... completed. Cool. So, should we have Bruce go ahead and pgindent now? Done and committed. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + None of us is going to be here forever. + -- 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] Re: [COMMITTERS] pgsql: Fix log_temp_files docs and comments to say bytes not kilobytes.
Robert Haas wrote: OK, so I talked to Bruce about this and I guess I've been persuaded that we should just apply the patch I sent upthread to HEAD and leave the back-branches broken, for fear of creating an incompatibility. The only thing that might be appropriate to backport is the docs fix, change kilobytes to bytes in config.sgml where the parameter is described. The bug complaint here was mad at the documentation not matching the behavior, rather than the behavior itself. I agree with the change to HEAD you've suggested though, that's the right way to do this for future releases. This change is something worth mentioning in the release notes for 9.0 too. -- Greg Smith 2ndQuadrant US Baltimore, MD PostgreSQL Training, Services and Support g...@2ndquadrant.com www.2ndQuadrant.us -- 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] t_self as system column
Excerpts from Robert Haas's message of mar jul 06 10:08:21 -0400 2010: Treating OID as a user-defined column seems reasonable, and probably not even that messy if we put some appropriate macros in place. I'm guessing the messy part would be finding all the places that expect to be consulting a real pg_attribute row and supplying them with a faked-up one in its place. Agreed. I'm intending to work on logical column identifiers for 9.1. Perhaps I could try to have a look at this, too, while at it. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Fix log_temp_files docs and comments to say bytes not kilobytes.
On Tue, Jul 6, 2010 at 3:40 PM, Greg Smith g...@2ndquadrant.com wrote: Robert Haas wrote: OK, so I talked to Bruce about this and I guess I've been persuaded that we should just apply the patch I sent upthread to HEAD and leave the back-branches broken, for fear of creating an incompatibility. The only thing that might be appropriate to backport is the docs fix, change kilobytes to bytes in config.sgml where the parameter is described. The bug complaint here was mad at the documentation not matching the behavior, rather than the behavior itself. I agree with the change to HEAD you've suggested though, that's the right way to do this for future releases. This change is something worth mentioning in the release notes for 9.0 too. I talked about that with Bruce. It wouldn't actually be sufficient to just say it's in bytes, because if you do show log_temp_files, it'll tell you that it's in kB, but it's really not. So we'd need to basically explain that it's a bug we're not fixing for fear of breaking existing installations. Bruce felt it wasn't worth putting that amount of work into backbranch docs that nobody's likely to read anyway, but I suppose that view could be overruled if there's a strong consensus. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres 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] t_self as system column
On Tue, Jul 6, 2010 at 2:49 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: Excerpts from Robert Haas's message of mar jul 06 10:08:21 -0400 2010: Treating OID as a user-defined column seems reasonable, and probably not even that messy if we put some appropriate macros in place. I'm guessing the messy part would be finding all the places that expect to be consulting a real pg_attribute row and supplying them with a faked-up one in its place. Agreed. I'm intending to work on logical column identifiers for 9.1. Perhaps I could try to have a look at this, too, while at it. I have a strong suspicion that's going to be a, ahem, challenging project. But it would be great to have. Getting rid of the system column entries from pg_attribute is probably easy by comparison. When we discussed this previously, Tom suggested that we might want to have a three-tiered structure: (1) permanent identifier (never changes, used by other system catalogs to reference the attribute in question), (2) display position, and (3) physical storage position. I'm not sure if it's feasible to think about splitting out (2) and (3) in a single patch, but either one would be useful by itself. Which are you planning to work on? One other thought for you to mull over. Currently, we can never really totally get rid of an attribute because it would leave us at a loss as to how to interpret the tuples already on disk - we can't cope with HeapTupleHeaderGetNatts ever decreasing. But maybe instead of storing natts in the tuple header, we could store a tuple version number. Whenever a column is added or dropped, physical storage layout is changed, etc., we bump the tuple version number but retain the information necessary to interpret old tuple versions. After CLUSTER or VACUUM FULL, we can forget about all the old tuple versions. A regular VACUUM can, if it visits the entire table, forget about all tuple versions other than the latest which are observed not to be in use. It's a bit awkward if you go to make a change and discover that all 2047 possible tuple versions are in use, because now you have to force a table rewrite for an operation that doesn't normally require one. But in the immortal words of Bill Gates, 640K ought to be enough for anybody. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres 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] t_self as system column
Robert Haas wrote: On Tue, Jul 6, 2010 at 2:49 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: I'm intending to work on logical column identifiers for 9.1. Perhaps I could try to have a look at this, too, while at it. I have a strong suspicion that's going to be a, ahem, challenging project. But it would be great to have. Getting rid of the system column entries from pg_attribute is probably easy by comparison. It will be a bit invasive, but I'm not so sure that it's difficult, just a mass of details to take care of. Like you I'd be very glad to see it done. When we discussed this previously, Tom suggested that we might want to have a three-tiered structure: (1) permanent identifier (never changes, used by other system catalogs to reference the attribute in question), (2) display position, and (3) physical storage position. I'm not sure if it's feasible to think about splitting out (2) and (3) in a single patch, but either one would be useful by itself. Which are you planning to work on? Why wouldn't it be feasible? In any case, having a mutable logical column position is the feature that's been most requested. cheers andrew -- 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] keepalives on MacOS X
On Fri, Jul 2, 2010 at 5:44 PM, Robert Haas robertmh...@gmail.com wrote: On Tue, Jun 29, 2010 at 10:53 PM, Fujii Masao masao.fu...@gmail.com wrote: On Tue, Jun 29, 2010 at 11:28 PM, Robert Haas robertmh...@gmail.com wrote: On Tue, Jun 29, 2010 at 12:42 AM, Tom Lane t...@sss.pgh.pa.us wrote: Yeah, a bit of rooting around in the Darwin sources shows that this value is used as a per-connection override for tcp_keepidle. So it is a synonym. Not sure if it's worth supporting when the other value can't be set too. Maybe it'd be more useful to document that people can set the system-wide values if they're desperate. Well, the default value for tcp_keepidle is 2 hours, and the default value for tcp_keepintvl is 75 seconds. Assuming that tcp_keepcount defaults to something reasonable (I think the default on Linux is 9), the lion's share of the time will be waiting for tcp_keepidle - so just the ability to reduce that value to something reasonable should help a lot. It's also not much code - proposed patch attached. src/interfaces/libpq/fe-connect.c + appendPQExpBuffer(conn-errorMessage, + libpq_gettext(setsockopt(TCP_KEEPIDLE) failed: %s\n), + SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf))); s/TCP_KEEPIDLE/TCP_KEEPALIVE Fixed. Don't we need to change pq_getkeepalivesidle? I changed this, but it doesn't seem to have done much. When I do show tcp_keepalives_idle on MacOS X, I still get 0. gdb says getsockopt is getting called, though. Am I doing something boneheaded here, or is this just the behavior? In pq_setkeepalivesidle, if neither TCP_KEEPIDLE nor TCP_KEEPALIVE are supported, the following message is output. setsockopt(TCP_KEEPIDLE) not supported Fixed. Committed, after some further testing. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres 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] t_self as system column
On Tue, Jul 6, 2010 at 5:18 PM, Andrew Dunstan and...@dunslane.net wrote: I have a strong suspicion that's going to be a, ahem, challenging project. But it would be great to have. Getting rid of the system column entries from pg_attribute is probably easy by comparison. It will be a bit invasive, but I'm not so sure that it's difficult, just a mass of details to take care of. Like you I'd be very glad to see it done. I guess we'll find out...! When we discussed this previously, Tom suggested that we might want to have a three-tiered structure: (1) permanent identifier (never changes, used by other system catalogs to reference the attribute in question), (2) display position, and (3) physical storage position. I'm not sure if it's feasible to think about splitting out (2) and (3) in a single patch, but either one would be useful by itself. Which are you planning to work on? Why wouldn't it be feasible? Just because it might be too much to do all at once. In any case, having a mutable logical column position is the feature that's been most requested. I think that's true. But the physical storage position would give us a performance benefit, by allowing us to try to avoid useless alignment padding. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres 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] t_self as system column
Robert Haas robertmh...@gmail.com writes: On Tue, Jul 6, 2010 at 5:18 PM, Andrew Dunstan and...@dunslane.net wrote: Why wouldn't it be feasible? Just because it might be too much to do all at once. My thought is that the hardest part of this is going to be making sure that every column index usage in the code is properly categorized as to whether it's physical, logical, or identifier index. If we try to divide the problem into sub-patches, that will probably just increase the amount of effort because all that code will have to be looked at twice. Think of it as Polya's paradox in action. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Does mbutils.c really need to use L'\0' ?
I grow weary of mopping up after pgindent, as in http://archives.postgresql.org/pgsql-committers/2010-07/msg00069.php The problem evidently is that pgindent hasn't heard of wide character constants. No doubt the best fix would be to teach it about them; but given that we seem to have next to no use for such constants, I'm dubious that it's worth the trouble. I suggest that it might be best to replace these usages of L'\0' by plain scalar 0. Does anyone think that wouldn't work or is too grotty? 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] Re: [COMMITTERS] pgsql: Fix log_temp_files docs and comments to say bytes not kilobytes.
Robert Haas wrote: Bruce felt it wasn't worth putting that amount of work into backbranch docs that nobody's likely to read anyway, but I suppose that view could be overruled if there's a strong consensus. I was never arguing in favor of touching anything in the back branches; if you recall I didn't even voice an opinion here until I got concerned about too many changes happening in them. I think a proper fix in 9.0 combined with a release notes comment noting the old/new behavior, so it's clear what was broken in the old versions, would be quite enough here. Something like this maybe: Change log_temp_files to be set in and output its value in kilobytes. Older versions had the parameter labeled and documented as being in KB, while it was actually set and outputting to the logs in bytes. Thanks for following this through, I think it's a useful small bit to get sorted out fully. -- Greg Smith 2ndQuadrant US Baltimore, MD PostgreSQL Training, Services and Support g...@2ndquadrant.com www.2ndQuadrant.us -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add note that using PL/Python 2 and 3 in the same session will
pet...@postgresql.org (Peter Eisentraut) writes: Log Message: --- Add note that using PL/Python 2 and 3 in the same session will probably crash Crash? I can see people regarding that as a security problem. Maybe we need to do something more pro-active to prevent such conflicts? 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] [COMMITTERS] pgsql: Add note that using PL/Python 2 and 3 in the same session will
On tis, 2010-07-06 at 17:49 -0400, Tom Lane wrote: pet...@postgresql.org (Peter Eisentraut) writes: Log Message: --- Add note that using PL/Python 2 and 3 in the same session will probably crash Crash? I can see people regarding that as a security problem. Maybe we need to do something more pro-active to prevent such conflicts? I don't see how. Loading any module that uses the same symbols as another already loaded modules can cause the same problem. -- 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] Does mbutils.c really need to use L'\0' ?
Tom Lane wrote: I grow weary of mopping up after pgindent, as in http://archives.postgresql.org/pgsql-committers/2010-07/msg00069.php The problem evidently is that pgindent hasn't heard of wide character constants. No doubt the best fix would be to teach it about them; but given that we seem to have next to no use for such constants, I'm dubious that it's worth the trouble. I suggest that it might be best to replace these usages of L'\0' by plain scalar 0. Does anyone think that wouldn't work or is too grotty? or maybe 0x, which I gather from http://en.wikipedia.org/wiki/Wide_character is the usual locution. cheers andrew. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add note that using PL/Python 2 and 3 in the same session will
On Tue, 2010-07-06 at 17:49 -0400, Tom Lane wrote: pet...@postgresql.org (Peter Eisentraut) writes: Log Message: --- Add note that using PL/Python 2 and 3 in the same session will probably crash Crash? I can see people regarding that as a security problem. Maybe we need to do something more pro-active to prevent such conflicts? +1 Joshua D. Drake -- PostgreSQL.org Major Contributor Command Prompt, Inc: http://www.commandprompt.com/ - 509.416.6579 Consulting, Training, Support, Custom Development, Engineering -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add note that using PL/Python 2 and 3 in the same session will
Peter Eisentraut pete...@gmx.net writes: On tis, 2010-07-06 at 17:49 -0400, Tom Lane wrote: Crash? I can see people regarding that as a security problem. Maybe we need to do something more pro-active to prevent such conflicts? I don't see how. Loading any module that uses the same symbols as another already loaded modules can cause the same problem. The scenario that worries me is that as soon as somebody has created both plpython2 and plpython3 functions in the same database, he's at risk of crashes or maybe worse, even if the functions weren't intended to be used together. I don't believe that the problem exists, or at least is of anywhere near the same scope, for ordinary loadable modules. As was noted earlier, ordinary modules don't have references to each other, which is why RTLD_LOCAL would be a good choice if it weren't for Python. (I assume that a sane linker will resolve a module's references to itself if possible, even if there are conflicts elsewhere.) The reason why Python has got an issue here is that it has such heavy dependence on add-on loadable modules, which have references to the core python.so library. So with two python.so versions loaded, you don't know which one an add-on module's symbol references will be resolved to. (I wonder if that would be fixable with some better use of symbol versioning? But that's something for the Python maintainers not us.) At this point it seems clear to me that we've not adequately thought through the implications of having two python versions in one application namespace, and I'm not sure the Python people have either. I think we need to do something to block that from happening, at least until we have a plausible way to make it work. 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] Does mbutils.c really need to use L'\0' ?
Andrew Dunstan and...@dunslane.net writes: Tom Lane wrote: I'm dubious that it's worth the trouble. I suggest that it might be best to replace these usages of L'\0' by plain scalar 0. Does anyone think that wouldn't work or is too grotty? or maybe 0x, which I gather from http://en.wikipedia.org/wiki/Wide_character is the usual locution. Hm. I don't really read that page as suggesting that 0x is what to use if your compiler hasn't got wide chars. I'd tend to go with just 0, which is a reasonably common substitute for non-wide '\0' ... 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] Re: [COMMITTERS] pgsql: Fix log_temp_files docs and comments to say bytes not kilobytes.
On Tue, Jul 6, 2010 at 5:49 PM, Greg Smith g...@2ndquadrant.com wrote: I was never arguing in favor of touching anything in the back branches; if you recall I didn't even voice an opinion here until I got concerned about too many changes happening in them. I think a proper fix in 9.0 combined with a release notes comment noting the old/new behavior, so it's clear what was broken in the old versions, would be quite enough here. OK, commit done in head, with a note that we're deliberately not touching the back-branches and should release-note the change. Open item removed, also. Thanks for following this through, I think it's a useful small bit to get sorted out fully. yw -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres 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] Proposal for 9.1: WAL streaming from WAL buffers
On Fri, Jun 11, 2010 at 9:14 AM, Fujii Masao masao.fu...@gmail.com wrote: In 9.0, walsender reads WAL always from the disk and sends it to the standby. That is, we cannot send WAL until it has been written (and flushed) to the disk. This degrades the performance of synchronous replication very much since a transaction commit must wait for the WAL write time *plus* the replication time. The attached patch enables walsender to read data from WAL buffers in addition to the disk. Since we can write and send WAL simultaneously, in synchronous replication, a transaction commit has only to wait for either of them. So the performance would significantly increase. To recap the previous discussion on this thread, we ended up changing the behavior of 9.0 so that it only sends WAL which has been written to the OS *and flushed*, because sending unflushed WAL to the standby is unsafe. The standby can get ahead of the master while still believing that the databases are in sync, due to the fact that after an SR reconnect we rewind to the start of the current WAL segment. This results in a silently corrupt standby database. If it's unsafe to send written but unflushed WAL to the standby, then for the same reasons we can't send unwritten WAL either. Therefore, I believe that this entire patch in its current form is a nonstarter and we should mark it Rejected in the CF app so that reviewers don't unnecessarily spend time on it. Having said that, I do think we urgently need some high-level design discussion on how sync rep is actually going to handle this issue (perhaps on a new thread). If we can't resolve this issue, sync rep is going to be really slow; but there are no easy solutions to this problem in sight, so if we want to have sync rep for 9.1 we'd better agree on one of the difficult solutions soon so that work can begin. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres 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] Bug? Concurrent COMMENT ON and DROP object
(2010/07/06 23:33), Tom Lane wrote: Robert Haasrobertmh...@gmail.com writes: 2010/7/6 KaiGai Koheikai...@ak.jp.nec.com: In the following scenario, we can see orphan comments. Yeah. I think the reason we haven't seen any complaints about this before is that the worst-case scenario is that a comment for a dropped database object eventually becomes associated with a new database object. Well, in general there is very little DDL locking for any object type other than tables. I think the original rationale for that was that most other object types are defined by single catalog entries, so that attempts to update/delete the object would naturally block on changing its tuple anyway. But between comments and pg_depend entries that seems not particularly true anymore. IIRC there is now some attempt to lock objects of all types during DROP. Maybe the COMMENT code could acquire a conflicting lock. Are you saying AcquireDeletionLock()? It seems to me fair enough to prevent the problem, although it is declared as a static function. For example, we need to acquire a lock on the pg_type catalog when we try to comment on any type object. Perhaps, I think LockRelationOid() should be injected at head of the CommentType() in this case. Any comments? A more fine-grained lock would be preferable, s/preferable/essential/. This cure would be *far* worse than the disease. Can you say deadlock? regards, tom lane -- KaiGai Kohei kai...@ak.jp.nec.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] Bug? Concurrent COMMENT ON and DROP object
2010/7/6 KaiGai Kohei kai...@ak.jp.nec.com: (2010/07/06 23:33), Tom Lane wrote: Robert Haasrobertmh...@gmail.com writes: 2010/7/6 KaiGai Koheikai...@ak.jp.nec.com: In the following scenario, we can see orphan comments. Yeah. I think the reason we haven't seen any complaints about this before is that the worst-case scenario is that a comment for a dropped database object eventually becomes associated with a new database object. Well, in general there is very little DDL locking for any object type other than tables. I think the original rationale for that was that most other object types are defined by single catalog entries, so that attempts to update/delete the object would naturally block on changing its tuple anyway. But between comments and pg_depend entries that seems not particularly true anymore. IIRC there is now some attempt to lock objects of all types during DROP. Maybe the COMMENT code could acquire a conflicting lock. Are you saying AcquireDeletionLock()? It seems to me fair enough to prevent the problem, although it is declared as a static function. Obviously not. We don't need to acquire an AccessExclusiveLock to comment on an object - just something that will CONFLICT WITH an AccessExclusiveLock. So, use the same locking rules, perhaps, but take a much weaker lock, like AccessShareLock. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres 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] Bug? Concurrent COMMENT ON and DROP object
Robert Haas robertmh...@gmail.com writes: Obviously not. We don't need to acquire an AccessExclusiveLock to comment on an object - just something that will CONFLICT WITH an AccessExclusiveLock. So, use the same locking rules, perhaps, but take a much weaker lock, like AccessShareLock. Well, it probably needs to be a self-conflicting lock type, so that two COMMENTs on the same object can't run concurrently. But I agree AccessExclusiveLock is too strong: that implies locking out read-only examination of the object, which we don't want. 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] Bug? Concurrent COMMENT ON and DROP object
On Tue, Jul 6, 2010 at 10:18 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: Obviously not. We don't need to acquire an AccessExclusiveLock to comment on an object - just something that will CONFLICT WITH an AccessExclusiveLock. So, use the same locking rules, perhaps, but take a much weaker lock, like AccessShareLock. Well, it probably needs to be a self-conflicting lock type, so that two COMMENTs on the same object can't run concurrently. But I agree AccessExclusiveLock is too strong: that implies locking out read-only examination of the object, which we don't want. Hmm... so, maybe ShareUpdateExclusiveLock? That looks to be the weakest thing that is self-conflicting. The others are ShareRowExclusiveLock, ExclusiveLock, and AccessExclusiveLock. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres 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] Bug? Concurrent COMMENT ON and DROP object
Excerpts from Robert Haas's message of mar jul 06 22:31:40 -0400 2010: On Tue, Jul 6, 2010 at 10:18 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: Obviously not. We don't need to acquire an AccessExclusiveLock to comment on an object - just something that will CONFLICT WITH an AccessExclusiveLock. So, use the same locking rules, perhaps, but take a much weaker lock, like AccessShareLock. Well, it probably needs to be a self-conflicting lock type, so that two COMMENTs on the same object can't run concurrently. But I agree AccessExclusiveLock is too strong: that implies locking out read-only examination of the object, which we don't want. Hmm... so, maybe ShareUpdateExclusiveLock? So COMMENT ON will conflict with (auto)vacuum? Seems a bit weird ... -- 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] Does mbutils.c really need to use L'\0' ?
Tom Lane t...@sss.pgh.pa.us wrote: I'm dubious that it's worth the trouble. I suggest that it might be best to replace these usages of L'\0' by plain scalar 0. I'd tend to go with just 0, which is a reasonably common substitute for non-wide '\0' ... I think all of the following codes work in the same way at least on Windows, where the codes are actually used. utf16[dstlen] = L'\0'; utf16[dstlen] = '\0'; utf16[dstlen] = 0; utf16[dstlen] = (WCHAR) 0; Regards, --- Takahiro Itagaki NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] multibyte-character aware support for function downcase_truncate_identifier()
Hi All,
Every identifier is downcase truncated by function
downcase_truncate_identifier()
before using it.
But since the function downcase_truncate_identifier() is not
multibyte-charecter aware,
it is not able to downcase some of special charecters in identifier like
my_SchemÄ.
If schema is created of name my_SchemÄ, pg_namespace shows entries as
my_schemÄ .
Example is as below :
postgres=# create schema my_SchemÄ;
CREATE SCHEMA
postgres=# select nspname from pg_namespace;
nspname
pg_toast
pg_temp_1
pg_toast_temp_1
pg_catalog
public
information_schema
my_schemÄ
(7 rows)
postgres=#
Achually it should downcase as my_schemä as per multibyte-character aware
as lower()
works :
postgres=# select lower('my_SchemÄ');
lower
---
my_schemä
(1 row)
There is function str_tolower() which work as multibyte-character aware.
Need to use same function where ever downcase required. So, it will create
uniform down-casing at all places.
two places identified where need to add wide-character aware downcase :
1. downcase_truncate_identifier();
- Attaching patch for changes and small test case.
Following functions should also synchronise with
downcase_truncate_identifier() :
2. pg_strcasecmp();
3. pg_strncasecmp();
- to add fix at these functions (2,3) need to move str_tolower() from
formatting.c from backend to some common location (may be in src/port) from
where these can be used with client as well as server.
Thanks Regards,
Rajanikant Chirmade.
diff --git a/orig/postgresql-9.0beta2/src/backend/parser/scansup.c b/postgresql-9.0beta2/src/backend/parser/scansup.c
index 94082f7..179b37e 100644
--- a/orig/postgresql-9.0beta2/src/backend/parser/scansup.c
+++ b/postgresql-9.0beta2/src/backend/parser/scansup.c
@@ -129,33 +129,11 @@ char *
downcase_truncate_identifier(const char *ident, int len, bool warn)
{
char *result;
- int i;
-
- result = palloc(len + 1);
-
- /*
- * SQL99 specifies Unicode-aware case normalization, which we don't yet
- * have the infrastructure for. Instead we use tolower() to provide a
- * locale-aware translation. However, there are some locales where this
- * is not right either (eg, Turkish may do strange things with 'i' and
- * 'I'). Our current compromise is to use tolower() for characters with
- * the high bit set, and use an ASCII-only downcasing for 7-bit
- * characters.
- */
- for (i = 0; i len; i++)
- {
- unsigned char ch = (unsigned char) ident[i];
- if (ch = 'A' ch = 'Z')
- ch += 'a' - 'A';
- else if (IS_HIGHBIT_SET(ch) isupper(ch))
- ch = tolower(ch);
- result[i] = (char) ch;
- }
- result[i] = '\0';
+ result = str_tolower(ident, len);
- if (i = NAMEDATALEN)
- truncate_identifier(result, i, warn);
+ if (len = NAMEDATALEN)
+ truncate_identifier(result, len, warn);
return result;
}
--- This tests if identifier with special charecters using wide-charecter aware downcase.
create schema my_SchemÄ;
--- Since we smash identifiers to lower we try to find schema name
--- by downcasing nspname.
select count(nspname) from pg_namespace where nspname=LOWER('my_SchemÄ');
drop schema my_SchemÄ;
wide-charecter_aware_downcase.out
Description: Binary data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
