Re: [PATCHES] Auto Partitioning Patch - WIP version 1
On Fri, 2007-03-30 at 12:28 +0530, NikhilS wrote: Please find attached the WIP version 1 of the auto partitioning patch. There was discussion on this a while back on -hackers at: http://archives.postgresql.org/pgsql-hackers/2007-03/msg00375.php Please note that this patch tries to automate the activities that currently are carried out manually. It does nothing fancy beyond that for now. There were a lot of good suggestions, I have noted them down but for now I have tried to stick to the initial goal of automating existing steps for providing partitioning. Things that this patch does: I think this patch is a reasonable first step and clearly written, but not yet ready for application to Postgres in this commit fest. I would say we need: * Clear explanation of the new syntax, with examples of each permutation so we can see how that would work. In light of recent discussions on -hackers we need to take a view on whether we should go with Gavin's suggested syntax or this syntax. * There are some additional syntax items I don't understand the need for. So these need to be explained. * I would be against using the term PARTITION BY since it is already a phrase that is part of the SQL Standard. Perhaps PARTITIONED BY? * We need regression tests for any new command syntax * No docs - that might be the same thing as the first item -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com PostgreSQL UK 2008 Conference: http://www.postgresql.org.uk -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Auto Partitioning Patch - WIP version 1
Hi Simon, On Fri, Mar 21, 2008 at 7:30 PM, Simon Riggs [EMAIL PROTECTED] wrote: On Fri, 2007-03-30 at 12:28 +0530, NikhilS wrote: Please find attached the WIP version 1 of the auto partitioning patch. There was discussion on this a while back on -hackers at: http://archives.postgresql.org/pgsql-hackers/2007-03/msg00375.php Please note that this patch tries to automate the activities that currently are carried out manually. It does nothing fancy beyond that for now. There were a lot of good suggestions, I have noted them down but for now I have tried to stick to the initial goal of automating existing steps for providing partitioning. Things that this patch does: I think this patch is a reasonable first step and clearly written, but not yet ready for application to Postgres in this commit fest. I would say we need: * Clear explanation of the new syntax, with examples of each permutation so we can see how that would work. In light of recent discussions on -hackers we need to take a view on whether we should go with Gavin's suggested syntax or this syntax. * There are some additional syntax items I don't understand the need for. So these need to be explained. * I would be against using the term PARTITION BY since it is already a phrase that is part of the SQL Standard. Perhaps PARTITIONED BY? * We need regression tests for any new command syntax * No docs - that might be the same thing as the first item -- Thanks for taking a look. But if I am not mistaken Gavin and co. are working on a much exhaustive proposal. In light of that maybe this patch might not be needed in the first place? I will wait for discussion and a subsequent collective consensus here, before deciding the further course of actions. Regards, Nikhils -- EnterpriseDB http://www.enterprisedb.com
Re: [PATCHES] Auto Partitioning Patch - WIP version 1
NikhilS wrote: * Clear explanation of the new syntax, with examples of each permutation so we can see how that would work. In light of recent discussions on -hackers we need to take a view on whether we should go with Gavin's suggested syntax or this syntax. * There are some additional syntax items I don't understand the need for. So these need to be explained. * I would be against using the term PARTITION BY since it is already a phrase that is part of the SQL Standard. Perhaps PARTITIONED BY? * We need regression tests for any new command syntax * No docs - that might be the same thing as the first item Thanks for taking a look. But if I am not mistaken Gavin and co. are working on a much exhaustive proposal. In light of that maybe this patch might not be needed in the first place? I will wait for discussion and a subsequent collective consensus here, before deciding the further course of actions. I think it is unwise to wait on Gavin for a more complex implemention --- we might end up with nothing for 8.4. As long as your syntax is compatible with whatever Gavin proposed Gavin can add on to your patch once it is applied. -- Bruce Momjian [EMAIL PROTECTED]http://momjian.us EnterpriseDB http://postgres.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Auto Partitioning Patch - WIP version 1
Bruce Momjian [EMAIL PROTECTED] writes: NikhilS wrote: Thanks for taking a look. But if I am not mistaken Gavin and co. are working on a much exhaustive proposal. In light of that maybe this patch might not be needed in the first place? I will wait for discussion and a subsequent collective consensus here, before deciding the further course of actions. I think it is unwise to wait on Gavin for a more complex implemention --- we might end up with nothing for 8.4. As long as your syntax is compatible with whatever Gavin proposed Gavin can add on to your patch once it is applied. It would be equally unwise to apply a stopgap patch if we're not certain it will be upward compatible with what we want to do later. I haven't been through the partitioning threads at all yet, but I think what we probably want to have when we emerge from commit fest is some consensus on what the road map is for partitioning. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Auto Partitioning Patch - WIP version 1
On Fri, 2008-03-21 at 20:15 +0530, NikhilS wrote: Thanks for taking a look. But if I am not mistaken Gavin and co. are working on a much exhaustive proposal. In light of that maybe this patch might not be needed in the first place? We should wait to apply, but not wait to discuss. Somebody must take the initiative. -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com PostgreSQL UK 2008 Conference: http://www.postgresql.org.uk -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Auto Partitioning Patch - WIP version 1
On Fri, 2008-03-21 at 11:18 -0400, Bruce Momjian wrote: I think it is unwise to wait on Gavin for a more complex implemention --- we might end up with nothing for 8.4. As long as your syntax is compatible with whatever Gavin proposed Gavin can add on to your patch once it is applied. The patch as stands does little apart from bring together many DDL statements into one. Partitioning is much much more than this so there seems little reason to think we should rush to commit this, especially before we get some good docs that explain what it does, and why. -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com PostgreSQL UK 2008 Conference: http://www.postgresql.org.uk -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Logging conflicted queries on deadlocks
ITAGAKI Takahiro [EMAIL PROTECTED] writes: Here is a patch to log conflicted queries on deadlocks. Queries are dumped at CONTEXT in the same sorting order as DETAIL messages. Those queries are picked from pg_stat_get_backend_activity, as same as pg_stat_activity, so that users cannot see other user's queries. Applied with revisions. It's a cute idea --- I first thought it couldn't work reliably because of race conditions, but actually we haven't aborted our own transaction at this point, so everyone else involved in the deadlock is still waiting and it should be safe to grab their activity strings. However there was still a big implementation problem, which is that looking at pg_stat_activity could deliver very stale results, not only about other backends' queries but even our own. The data for that comes from a snapshot that might have been taken much earlier in our transaction. I replaced the code you were using with a new pgstat.c entry point that delivers up-to-date info directly from the shared memory array. (It might be better to log all queries in the server log and mask them in the client response, but I'm not sure how to do it...) Yeah, that would be cute, but we don't have any way to deliver a different CONTEXT string to the client than we do to the log. We could generate duplicate messages that go only to the log but that seemed pretty ugly. In practice this definition is probably good enough. One thing that I worried about for a little bit is that you can imagine privilege-escalation scenarios. Suppose that the user is allowed to call some SECURITY DEFINER function that runs as superuser, and a deadlock occurs inside that. As the patch stands, every query involved in the deadlock will be reported, which might be undesirable. We could make the test use the outermost session user's ID instead of current ID, but that might only move the security risks someplace else. Thoughts? regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Logging conflicted queries on deadlocks
Tom Lane [EMAIL PROTECTED] writes: ITAGAKI Takahiro [EMAIL PROTECTED] writes: Here is a patch to log conflicted queries on deadlocks. Queries are dumped at CONTEXT in the same sorting order as DETAIL messages. Those queries are picked from pg_stat_get_backend_activity, as same as pg_stat_activity, so that users cannot see other user's queries. Applied with revisions. It's a cute idea --- I first thought it couldn't work reliably because of race conditions, but actually we haven't aborted our own transaction at this point, so everyone else involved in the deadlock is still waiting and it should be safe to grab their activity strings. There's no way the other transaction's timeout could fire while we're doing this is there? Are we still holding the lw locks at this point which would prevent that? One thing that I worried about for a little bit is that you can imagine privilege-escalation scenarios. Suppose that the user is allowed to call some SECURITY DEFINER function that runs as superuser, and a deadlock occurs inside that. As the patch stands, every query involved in the deadlock will be reported, which might be undesirable. We could make the test use the outermost session user's ID instead of current ID, but that might only move the security risks someplace else. Thoughts? Perhaps we should only do this if the current user's ID is the same as the outermost session user's ID? -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's 24x7 Postgres support! -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Logging conflicted queries on deadlocks
Gregory Stark [EMAIL PROTECTED] writes: There's no way the other transaction's timeout could fire while we're doing this is there? Are we still holding the lw locks at this point which would prevent that? Ah, reading the patch I see comments indicating that yes that's possible and no, we don't really care. So, ok. If the backend disappears entirely could the string be empty? Perhaps it would be safer to copy out st_activity inside the loop checking st_changecount? It is a really nice feature though. Note that there was an unrelated demand for just this info on one of the other lists just today. Thanks very much Itagaki-san! -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's On-Demand Production Tuning -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Logging conflicted queries on deadlocks
Gregory Stark [EMAIL PROTECTED] writes: Ah, reading the patch I see comments indicating that yes that's possible and no, we don't really care. So, ok. If the backend disappears entirely could the string be empty? Right, we'd be pointing at a BackendStatusArray entry that was now unused, or even perhaps recycled by a new session. That memory doesn't move, so we don't need to worry about picking up something that's not a status string at all, but worst case it could be not the string we want. I think the odds are pretty low though. Perhaps it would be safer to copy out st_activity inside the loop checking st_changecount? Don't think it would really help any --- the other backend could've aborted and changed its status string before we ever get to this code at all. It is a really nice feature though. Note that there was an unrelated demand for just this info on one of the other lists just today. Thanks very much Itagaki-san! That was my feeling --- the usefulness is high enough that a small probability of a wrong display is a small price to pay. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Logging conflicted queries on deadlocks
Gregory Stark [EMAIL PROTECTED] writes: Tom Lane [EMAIL PROTECTED] writes: One thing that I worried about for a little bit is that you can imagine privilege-escalation scenarios. Perhaps we should only do this if the current user's ID is the same as the outermost session user's ID? A conservative approach would be to report the query texts *only* in the server log and never to the client --- this would need a bit of klugery but seems doable. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Auto Partitioning Patch - WIP version 1
Hi, On Fri, Mar 21, 2008 at 9:23 PM, Tom Lane [EMAIL PROTECTED] wrote: Bruce Momjian [EMAIL PROTECTED] writes: NikhilS wrote: Thanks for taking a look. But if I am not mistaken Gavin and co. are working on a much exhaustive proposal. In light of that maybe this patch might not be needed in the first place? I will wait for discussion and a subsequent collective consensus here, before deciding the further course of actions. I think it is unwise to wait on Gavin for a more complex implemention --- we might end up with nothing for 8.4. As long as your syntax is compatible with whatever Gavin proposed Gavin can add on to your patch once it is applied. It would be equally unwise to apply a stopgap patch if we're not certain it will be upward compatible with what we want to do later. I haven't been through the partitioning threads at all yet, but I think what we probably want to have when we emerge from commit fest is some consensus on what the road map is for partitioning. +2 Regards, Nikhils -- EnterpriseDB http://www.enterprisedb.com
Re: [PATCHES] Auto Partitioning Patch - WIP version 1
Hi all, I think the intent of the syntax / parser patch from Gavin and Jeff was to get consensus from PG on the syntax prior to proceeding with the next chunk of work. The next chunk of work is now well underway - with support for ALTER TABLE and partitioning, along with fast inserts into the parent table. This involves changes to the catalog, so we'll also need to discuss this as part of a submission. GP is in the middle of merging 8.3 into our product, so it will be a few weeks at least before we can push any more info to the list. Was there consensus on the syntax? IIRC, there was a cessation of contrary comments on the matter. If so, the parser patch was provided earlier - we could posibly refresh it. The way it works in our dev branch now is that the partition syntax is turned off by default using a GUC, but is fully functional wrt creating rules, etc. This allows for experimentation. - Original Message - From: [EMAIL PROTECTED] [EMAIL PROTECTED] To: Tom Lane [EMAIL PROTECTED] Cc: Bruce Momjian [EMAIL PROTECTED]; Simon Riggs [EMAIL PROTECTED]; pgsql-patches@postgresql.org pgsql-patches@postgresql.org Sent: Sat Mar 22 01:19:01 2008 Subject: Re: [PATCHES] Auto Partitioning Patch - WIP version 1 Hi, On Fri, Mar 21, 2008 at 9:23 PM, Tom Lane [EMAIL PROTECTED] wrote: Bruce Momjian [EMAIL PROTECTED] writes: NikhilS wrote: Thanks for taking a look. But if I am not mistaken Gavin and co. are working on a much exhaustive proposal. In light of that maybe this patch might not be needed in the first place? I will wait for discussion and a subsequent collective consensus here, before deciding the further course of actions. I think it is unwise to wait on Gavin for a more complex implemention --- we might end up with nothing for 8.4. As long as your syntax is compatible with whatever Gavin proposed Gavin can add on to your patch once it is applied. It would be equally unwise to apply a stopgap patch if we're not certain it will be upward compatible with what we want to do later. I haven't been through the partitioning threads at all yet, but I think what we probably want to have when we emerge from commit fest is some consensus on what the road map is for partitioning. +2 Regards, Nikhils -- EnterpriseDB http://www.enterprisedb.com
