Re: Portability concerns over pq_sendbyte?
On Wed, Jun 06, 2018 at 12:27:58PM -0400, Alvaro Herrera wrote: > Do you have an answer to this question? Does anybody else? > > (My guts tell me it'd be better to change these routines to take > unsigned values, without creating extra variants. But guts frequently > misspeak.) My guts are telling me as well to not have more variants. On top of that it seems to me that we'd want to rename any new routines to include "uint" in their name instead of "int", and for compatibility with past code pq_sendint should not be touched. -- Michael signature.asc Description: PGP signature
Re: Concurrency bug in UPDATE of partition-key
On Mon, Jun 11, 2018 at 10:19 AM, Amit Khandekar
wrote:
> On 8 June 2018 at 16:44, Dilip Kumar wrote:
> > On Fri, Jun 8, 2018 at 2:23 PM, Dilip Kumar
> wrote:
> >>
> >> On Tue, Jun 5, 2018 at 8:03 PM, Amit Khandekar
> >> wrote:
> >>>
> >>> Attached is a rebased patch version. Also included it in the upcoming
> >>> commitfest :
> >>> https://commitfest.postgresql.org/18/1660/
> >>>
> >>> In the rebased version, the new test cases are added in the existing
> >>> isolation/specs/partition-key-update-1.spec test.
> >>
> >>
> >> /*
> >> + * If this is part of an UPDATE of partition-key, the
> >> + * epq tuple will contain the changes from this
> >> + * transaction over and above the updates done by the
> >> + * other transaction. The caller should now use this
> >> + * tuple as its NEW tuple, rather than the earlier NEW
> >> + * tuple.
> >> + */
> >> + if (epqslot)
> >> + {
> >> + *epqslot = my_epqslot;
> >> + return NULL;
> >> + }
> >>
> >> I think we need simmilar fix if there are BR Delete trigger and the
> >> ExecDelete is blocked on heap_lock_tuple because the concurrent
> transaction
> >> is updating the same row. Because in such case it would have already
> got
> >> the final tuple so the hep_delete will return MayBeUpdated.
> >
> >
> > Amit Kapila had pointed (offlist) that you are already trying to fix this
> > issue.
>
> Yes, his comment about triggers made me realize the trigger issue
> which you mentioned, about which I also explained in earlier reply.
>
> > I have one more question, Suppose the first time we come for
> > ExecDelete and fire the BR delete trigger and then realize that we need
> to
> > retry because of the concurrent update. Now, during retry, we realize
> that
> > because of the latest value we don't need to route the tuple to the
> > different partition so now we will call ExecUpdate and may fire the
> BRUpdate
> > triggers as well?
>
> No, we don't fire the BR update trigger again. We go to lreplace
> label, and BR trigger code is above that label. The reason we don't
> need to run the trigger check again is because if we had already fired
> it before, the row must have been locked, so concurrency scenario
> won't arise in the first place.
Ok, that make sense.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Re: Concurrency bug in UPDATE of partition-key
On 8 June 2018 at 16:44, Dilip Kumar wrote:
> On Fri, Jun 8, 2018 at 2:23 PM, Dilip Kumar wrote:
>>
>> On Tue, Jun 5, 2018 at 8:03 PM, Amit Khandekar
>> wrote:
>>>
>>> Attached is a rebased patch version. Also included it in the upcoming
>>> commitfest :
>>> https://commitfest.postgresql.org/18/1660/
>>>
>>> In the rebased version, the new test cases are added in the existing
>>> isolation/specs/partition-key-update-1.spec test.
>>
>>
>> /*
>> + * If this is part of an UPDATE of partition-key, the
>> + * epq tuple will contain the changes from this
>> + * transaction over and above the updates done by the
>> + * other transaction. The caller should now use this
>> + * tuple as its NEW tuple, rather than the earlier NEW
>> + * tuple.
>> + */
>> + if (epqslot)
>> + {
>> + *epqslot = my_epqslot;
>> + return NULL;
>> + }
>>
>> I think we need simmilar fix if there are BR Delete trigger and the
>> ExecDelete is blocked on heap_lock_tuple because the concurrent transaction
>> is updating the same row. Because in such case it would have already got
>> the final tuple so the hep_delete will return MayBeUpdated.
>
>
> Amit Kapila had pointed (offlist) that you are already trying to fix this
> issue.
Yes, his comment about triggers made me realize the trigger issue
which you mentioned, about which I also explained in earlier reply.
> I have one more question, Suppose the first time we come for
> ExecDelete and fire the BR delete trigger and then realize that we need to
> retry because of the concurrent update. Now, during retry, we realize that
> because of the latest value we don't need to route the tuple to the
> different partition so now we will call ExecUpdate and may fire the BRUpdate
> triggers as well?
No, we don't fire the BR update trigger again. We go to lreplace
label, and BR trigger code is above that label. The reason we don't
need to run the trigger check again is because if we had already fired
it before, the row must have been locked, so concurrency scenario
won't arise in the first place.
--
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company
Re: CF bug fix items
On Sun, Jun 10, 2018 at 10:41:26AM -0400, Andrew Dunstan wrote: > Six of them are marked "ready for committer" and all of those have a > committer named as either author or reviewer. It would be good to get those > committed as soon as possible. So Heikki, Michael, Alexander, Simon, Etsuro > and Thomas, that means you :-) Pleas claim and commit those if possible, or > at least move them forward. Thanks for beginning this thread and summarizing the situation. > "Fix the optimization to skip WAL-logging on table created in same > transaction" has been in 10 (!) commitfests. It's seen no substantive action > since November. It has a bunch of authors and reviewers listed, Surely > somebody can move it forward? I think that this is a complicated topic, which results in a rather large and invasive patch introducing new logic concepts in order to fix a rather narrow use-case. So I am wondering if it is really something we ought to fix here.. > "Fix a bug that can prevent standby from restarting" has been around for 6 > CFs. This appears to be covered by commit > 0668719801838aa6a8bda330ff9b3d20097ea844. Can we just closed this one out? > If there's more work left to do another CF item could be added. Yes, I have marked it as committed. > Of the rest that have been around since January, > > "pg_rewind corrupts control file global/pg_control" discussion stopped in > April. Tom had made some comments to which Michael responded. Yeah, for this one I can see a bunch of approaches, and each one has unwelcome downsides, so if we were to patch something I think that I would go with the documentation-only patch. Something which could be considered for the future is to extend pg_stat_file so as it returns the umask of a file which could be used as a filter. The good thing is that we use stat() in pg_stat_file() which would return the info about the file a symlink points to. The thing is that we need to look at files which cannot be written locally as well as remotely. So it may be a lot of facility for a small use-case, still that could be an interesting set of two TODO items (one for pg_rewind, one to extend pg_stat_file). > "Produce a crash dump before main() on Windows", discussion stopped in > March. So the commit fest entry is here: https://commitfest.postgresql.org/18/1525/ Noah has committed a portion of things with cbfffee4. Craig, as you already looked at the first patch, perhaps you could check the latest version? Or Noah as you looked at the thread? "Cascaded standby cannot start after a shutdown" which is here is something I already reviewed and proposed a solution for, so I could look at that again: https://commitfest.postgresql.org/18/1516/ "Failure at replay for corrupted 2PC files + reduce window between end-of-recovery record and history file write" is also something worrying me per the way we simply ignore on-disk 2PC files which are corrupted in the data directory. So I could look at this one if there are no objections. The last set of patches is here as well: https://www.postgresql.org/message-id/CAB7nPqT9V6N7Wm7npkqcHf-beLtZ82z2dNG_5cwfBLg%3Dm5PfUw%40mail.gmail.com -- Michael signature.asc Description: PGP signature
Re: commitfest 2018-07
On Wed, Jun 06, 2018 at 12:38:56AM +0200, Magnus Hagander wrote: > Thus, if we don't want to have to risk doing surgery on the system (or > guarantee we won't bounce any patches from the 07 CF, but that seems like > the wrong thing to do), we should rename the existing 09 CF to 07, so all > patches automatically end up there, and stick to only "bouncing patches in > the forward direction". So, we have confirmation from the RTM that there will be a 2018-07. And there is visibly consensus that renaming 2018-09 to 2018-07 makes the most sense. Any objections in moving forward with this renaming at the 5-CF plan for v12? -- Michael signature.asc Description: PGP signature
Re: why partition pruning doesn't work?
Thanks for working on and pushing those fixes. On 11 June 2018 at 10:49, Tom Lane wrote: > It's very unclear for example what the subplan_map and subpart_map > arrays really are, eg what are they indexed by? I get the impression > that only one of them can have a non-minus-1 value for a given index, > but that's nowhere explained. Also, we have They're indexed by the partition indexes as returned by the partition pruning code. I've included a patch to fix this. > * partprunedataArray of PartitionPruningData for the plan's target > * partitioned relation. First element contains the > * details for the target partitioned table. > > And? What are the other elements, what's the index rule, is there a > specific ordering for the other elements? For that matter, "target > partitioned table" is content-free. Do you mean topmost partitioned > table? I suspect we expect the hierarchy to be flattened such that > ancestors appear before children, but that's not stated --- and if it > were, this bit about the first element would be a consequence of that. I've hopefully improved this comment in the attached patch. Although we may need to delay this until [1] has been addressed Only the index of the first element is important. Looking at add_paths_to_append_rel() the parents will always be listed before their children. That's not very well documented either, but having the top-level parent as the first element is relied upon for more than runtime partition pruning. > Code-wise, there are some loose ends to be looked at. > > * Related to the above, doesn't the loop at execPartition.c:1658 need > to work back-to-front? Seems like it's trying to propagate info upwards > in the hierarchy; looking at a subpartition's present_parts value when > you still might change it later doesn't look right at all. It works just fine front-to-back. That's why I added the 2nd loop. It propagates the work done in the first loop, so the code, as it is now, works if the array is in any order. It may be possible to run the first loop back-to-front and get rid of the 2nd one. I've done this in the attached patch. I felt less confident doing this earlier as the order of that array was not defined well. > * partkey_datum_from_expr and its caller seem pretty brain-dead with > respect to nulls. It's not even considering the possibility that a > Const has constisnull = true. Now perhaps such a case can't reach > here because of plan-time constant-folding, but I don't think this code > has any business assuming that. It's also being very stupid about > null values from expressions, just throwing up its hands and supposing > that nothing can be proven. In reality, doesn't a null guarantee we > can eliminate all partitions, since the comparison functions are > presumed strict? You are right. I admit to not having thought more about that and just taking the safe option. It makes sense to fix it. I imagine it could be a bit annoying that pruning just gives up in such a case. I've added code for this in the attached patch. > * I'm fairly suspicious of the fmgr_info and fmgr_info_copy calls in > perform_pruning_base_step. Those seem likely to leak memory, and > for sure they destroy any opportunity for the called comparison > function to cache info in fn_extra --- something that's critical > for performance for some comparison functions such as array_eq. > Why is it necessary to suppose that those functions could change > from one execution to the next? IIRC the problem there was that we didn't think of a good place to cache the FmgrInfo. But rethinking of that now we could probably use the same method as I used in run-time pruning to cache the exprstate. Amit, what do you think? > * The business with ExecFindInitialMatchingSubPlans remapping the > subplan indexes seems very dubious to me. Surely, that is adding > way more complexity and fragility than it's worth. It's all contained in a single .c file. It does not seem that hard to get it right. Nothing outside of exexPartition.c has any business changing anything in these arrays. Nothing else even has to look at them. I think having the ability to prune during executor initialisation is enormously important. I think without it, this patch is less than half as useful. However, if you didn't mean removing the executor initialise pruning, and just not remapping the arrays, then I'm not sure how we'd do that. I'd previously tried having NULL subnodes in the Append and I thought it required too much hacking work in explain.c, so I decided to just collapse the array instead and do what was required to make it work after having removed the unneeded subplans. Did you have another idea on how to do this? [1] https://www.postgresql.org/message-id/CAKJS1f9KG5nnOFhigWm4ND5Ut-yU075iJyA%2BACVyQnZ-ZW1Qtw%40mail.gmail.com -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support,
Re: SHOW ALL does not honor pg_read_all_settings membership
On Fri, Jun 08, 2018 at 03:13:57PM -0400, Alvaro Herrera wrote: > And I think this fixes it. -if (conf->source == PGC_S_FILE && superuser()) +if (conf->source == PGC_S_FILE && +is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_SETTINGS)) Thanks Alvaro! This bit in GetConfigOptionByNum() looks fine to me. -- Michael signature.asc Description: PGP signature
Re: Loaded footgun open_datasync on Windows
On Sun, Jun 10, 2018 at 10:09:26AM +0530, Amit Kapila wrote: > As per discussion till now, we have two options to proceed: > (a) Remove the define "#ifndef FRONTEND" that prevents pgwin32_open > usage in frontend modules. We have done some research and found that > it was added in the past to allow build of modules like libpq/psql. > If we want to use this option, then some work is needed to ensure all > frontend modules work/behave correctly. > (b) Use c.h in pg_test_fsync which will allow usage of pgwin32_open. > Option (a) appears to be a good approach, but I am not sure what exact > tests would be required. pg_upgrade could be a good match here by removing the stuff around SERVER_START_LOG_FILE and SERVER_STOP_LOG_FILE in pg_upgrade.h and that can run with a single command using vcregress.bat. > Do you prefer any of the above or have any better suggestions? (a) looks like a good plan to me, as long as there is no back-patch as the result patch would be likely invasive. I would also suggest for v12 to open for commits. -- Michael signature.asc Description: PGP signature
Re: pgsql: Fix and document lock handling for in-memory replication slot da
On Sun, Jun 10, 2018 at 10:45:04AM +, Michael Paquier wrote: > Fix and document lock handling for in-memory replication slot data > > [... snip ...] > > Details > --- > https://git.postgresql.org/pg/commitdiff/9e149c847f398793ec1641885434dcd10837d89d My apologies here. This commit has an outdated author timestamp, and I noticed it just after commit. I will be more careful in the future about that while cherry-picking changes across branches. I have as well upgraded by log configuration to that, which is mainly a matter of taste but it shows name and timestamp for both author and committer (%n stands for a newline, one configuration parameter should be on the same line as far as I know). [format] pretty = format:%C(blue)commit: %H%C(reset)%n %C(green)author: %aN <%aE>%C(reset)%n %C(green)date: %aD%C(reset)%n %C(yellow)committer: %cN <%ce>%C(reset)%n %C(yellow)date: %cD%C(reset)%n%B Feel free to reuse it, there are many other ways to set up that as well as git documentation says here: https://git-scm.com/docs/pretty-formats -- Michael signature.asc Description: PGP signature
Re: Possible bug in logical replication.
On Thu, Jun 07, 2018 at 08:32:10AM +0100, Simon Riggs wrote: > On 6 June 2018 at 17:22, Alvaro Herrera wrote: >> This thread seems to have died down without any fix being proposed. >> Simon, you own this open item. > > Thanks, will look. Petr and I have found a couple of issues about the slot advance stuff on this thread: https://www.postgresql.org/message-id/20180525052805.GA15634%40paquier.xyz The result is f731cfa, which, per my tests, is able to take care of this issue as well as advancing first a slot to a WAL page boundary, and then advancing it to the latest LSN available does not trigger any assertions anymore. It would be nice if there is a double-check though, so I am letting this thread on the list of open items for now. -- Michael signature.asc Description: PGP signature
Re: pg_replication_slot_advance to return NULL instead of 0/0 if slot not advanced
On Sun, Jun 10, 2018 at 10:19:23PM +0900, Michael Paquier wrote: > I have been able to look again at 0001 and pushed it as 9e149c8. As > reading inconsistent data from replication slots is rather hard to > trigger, I have just pushed the patch to HEAD. I'll look at 0002 > tomorrow. And pushed 0002 as f731cfa9, so we should be good with this open item. -- Michael signature.asc Description: PGP signature
Re: Internal error XX000 with enable_partition_pruning=on, pg 11 beta1 on Debian
David Rowley writes: > On 10 June 2018 at 04:48, Tom Lane wrote: >> So, IIUC, the issue is that for partitioning cases Append expects *all* >> its children to be partitions of the *same* partitioned table? That >> is, you could also break it with >> >> select * from partitioned_table_a >> union all >> select * from partitioned_table_b >> >> ? > Not quite. I think what I sent above is the most simple way to break > it. Your case won't because there are no quals to prune with, so > run-time pruning is never attempted. Well, I hadn't bothered to put in the extra code needed to have a qual to prune with, but my point remains that it doesn't seem like the current Append code is prepared to cope with an Append that contains partitions of more than one top-level partitioned table. I just had a thought that might lead to a nice solution to that, or might be totally crazy. What if we inverted the sense of the bitmaps that track partition pruning state, so that instead of a bitmap of valid partitions that need to be scanned, we had a bitmap of pruned partitions that we know we don't need to scan? (The indexes of this bitmap would be subplan indexes not partition indexes.) With this representation, it doesn't matter if some of the Append's children are not supposed to participate in pruning; they just don't ever get added to the bitmap of what to skip. It's also fairly clear, I think, how to handle independent pruning rules for different top-level tables that are being unioned together: just OR the what-to-skip bitmaps. But there may be some reason why this isn't workable. regards, tom lane
Re: why partition pruning doesn't work?
David Rowley writes: > I've made a pass over the execPartition.c and partprune.c code > attempting to resolve these issues. I have hopefully fixed them all, > but I apologise if I've missed any. > I also couldn't resist making a few other improvements to the code. By the time this arrived, I'd already whacked around your v4 patch quite a bit, so rather than start over I just kept going with what I had, and then tried to merge the useful bits of this one after the fact. I intentionally left out a couple of changes I couldn't get excited about (such as having find_subplans_for_params_recurse return a count), but I think I got everything in v5 otherwise. I'm still fairly unhappy about the state of the comments, though. It's very unclear for example what the subplan_map and subpart_map arrays really are, eg what are they indexed by? I get the impression that only one of them can have a non-minus-1 value for a given index, but that's nowhere explained. Also, we have * partprunedataArray of PartitionPruningData for the plan's target * partitioned relation. First element contains the * details for the target partitioned table. And? What are the other elements, what's the index rule, is there a specific ordering for the other elements? For that matter, "target partitioned table" is content-free. Do you mean topmost partitioned table? I suspect we expect the hierarchy to be flattened such that ancestors appear before children, but that's not stated --- and if it were, this bit about the first element would be a consequence of that. Code-wise, there are some loose ends to be looked at. * Related to the above, doesn't the loop at execPartition.c:1658 need to work back-to-front? Seems like it's trying to propagate info upwards in the hierarchy; looking at a subpartition's present_parts value when you still might change it later doesn't look right at all. * partkey_datum_from_expr and its caller seem pretty brain-dead with respect to nulls. It's not even considering the possibility that a Const has constisnull = true. Now perhaps such a case can't reach here because of plan-time constant-folding, but I don't think this code has any business assuming that. It's also being very stupid about null values from expressions, just throwing up its hands and supposing that nothing can be proven. In reality, doesn't a null guarantee we can eliminate all partitions, since the comparison functions are presumed strict? * I'm fairly suspicious of the fmgr_info and fmgr_info_copy calls in perform_pruning_base_step. Those seem likely to leak memory, and for sure they destroy any opportunity for the called comparison function to cache info in fn_extra --- something that's critical for performance for some comparison functions such as array_eq. Why is it necessary to suppose that those functions could change from one execution to the next? * The business with ExecFindInitialMatchingSubPlans remapping the subplan indexes seems very dubious to me. Surely, that is adding way more complexity and fragility than it's worth. regards, tom lane
JIT versus standalone-headers checks
I find that the JIT stuff has broken cpluspluscheck for me, along with a related script that I use to verify that each header builds cleanly standalone (ie with no prerequisites except postgres.h). There are two problems: (1) Doesn't work on a platform without the llvm header files: ./src/include/jit/llvmjit.h:18:26: error: llvm-c/Types.h: No such file or directory followed by a lot of complaints like ./src/include/jit/llvmjit.h:60: error: 'LLVMTypeRef' does not name a type It seems like a reasonable fix for that is to wrap the contents of these headers in "#ifdef USE_LLVM" ... do you see a reason not to? (2) This seems to need re-thought: ./src/include/jit/llvmjit.h:15:2: error: #error "llvmjit.h should only be included by code dealing with llvm" I don't especially see the value of this #error, especially if we are wrapping this whole header in "#ifdef USE_LLVM", and propose to just remove it. Thoughts? regards, tom lane
Re: PostgreSQL vs SQL Standard
On Sun, Jun 10, 2018 at 11:32:56AM -0400, Tom Lane wrote: > Andrew Gierth writes: > > I beat at the grammar a bit to see what it would take to fix it at least > > to the extent of allowing a_expr ColId in a select list after removing > > postfix ops. It looked like it was doable by making these keywords more > > reserved (all of which are already reserved words per spec): > > DOUBLE, DAY, FILTER, HOUR, MINUTE, MONTH, OVER, PRECISION, SECOND, > > VARYING, WITHIN, WITHOUT, YEAR > > Yeah, a side effect of allowing "a_expr ColId" is that we can expect, > going forward, that a lot of new keywords are going to have to become > fully reserved that otherwise wouldn't have to be. This is particularly > a problem because of the SQL committee's COBOL-hangover tendency to > invent new syntax that involves sequences of keywords; we usually > don't have a good way to deal with that short of making the first > keyword(s) reserved. > > It's arguable that going down that path will, in the long run, lead to > breaking more applications (via their table/column names suddenly becoming > reserved in some new version) than we rescue from having to quote their > SELECT aliases. At the very least we need to recognize that this is far > from cost-free. > > (wanders away wondering exactly what parsing technology the SQL committee > thinks implementations use...) If you're wondering why people think specifications that don't include a reference implementation are a bad idea... Best, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Re: assert in nested SQL procedure call in current HEAD
> On 8 June 2018 at 06:20, Andrew Gierth wrote: > Joe> My colleague Yogesh Sharma discovered an assert in nested SQL > Joe> procedure calls after ROLLBACK is used. Minimal test case and > Joe> backtrace below. I have not yet tried to figure out exactly what > Joe> is going on beyond seeing that it occurs in pg_plan_query() where > Joe> the comment says "Planner must have a snapshot in case it calls > Joe> user-defined functions"... > > Andrew> https://www.postgresql.org/message-id/29608.1518533...@sss.pgh.pa.us > > I added it to the open items list since nobody else seems to have taken > notice; from Tom's linked message it seems this should be Peter E's bag? I've taken a look at this - indeed, the situation looks similar to what described in the linked message, namely after a transaction rollback and creation of a new one no active snapshot was pushed. But in this particular case the timeframe without an active snapshot is actually limited and includes only some initialization and planning activity (after that a new one is pushed). The commentary says that "Planner must have a snapshot in case it calls user-defined functions." - I tried to simulate this in order to see what would happen, but got no errors. Is there a chance that it's an outdated Assert?
Re: PostgreSQL vs SQL Standard
Andrew Gierth writes: > I beat at the grammar a bit to see what it would take to fix it at least > to the extent of allowing a_expr ColId in a select list after removing > postfix ops. It looked like it was doable by making these keywords more > reserved (all of which are already reserved words per spec): > DOUBLE, DAY, FILTER, HOUR, MINUTE, MONTH, OVER, PRECISION, SECOND, > VARYING, WITHIN, WITHOUT, YEAR Yeah, a side effect of allowing "a_expr ColId" is that we can expect, going forward, that a lot of new keywords are going to have to become fully reserved that otherwise wouldn't have to be. This is particularly a problem because of the SQL committee's COBOL-hangover tendency to invent new syntax that involves sequences of keywords; we usually don't have a good way to deal with that short of making the first keyword(s) reserved. It's arguable that going down that path will, in the long run, lead to breaking more applications (via their table/column names suddenly becoming reserved in some new version) than we rescue from having to quote their SELECT aliases. At the very least we need to recognize that this is far from cost-free. (wanders away wondering exactly what parsing technology the SQL committee thinks implementations use...) regards, tom lane PS: My older message about this mentioned only the VARYING and ISNULL cases, which I think means that I had ideas about how not to have to reserve the other ones you mention. But the larger point holds.
Re: PostgreSQL vs SQL Standard
Andrew Gierth writes: > Oh wow, I hadn't noticed that dropping a function referenced from a > domain's default or constraint drops the whole domain rather than just > removing the default or constraint the way it would with a table. Ouch. Seems like possibly a bug ... shouldn't we make only that constraint depend on the function? But that's orthogonal to the DROP DOMAIN behavior you were describing. > (If it were not the case, then the only way we'd end up cascading to > dropping a domain would be if we dropped the base type, in which case > the columns are going to go away anyway) Nope, drop schema and drop owned by (at the least) could also cascade to a domain. regards, tom lane
CF bug fix items
I've been looking over the older items in the CF, specifically those in the "bug fix" category. Six of them are marked "ready for committer" and all of those have a committer named as either author or reviewer. It would be good to get those committed as soon as possible. So Heikki, Michael, Alexander, Simon, Etsuro and Thomas, that means you :-) Pleas claim and commit those if possible, or at least move them forward. In one of those cases, "ConvertRowtypeExpr reference errors from partition-wise join", the patch has been marked Ready for Committer and then Etsuro seems to have changeed his mind. If it's not ready it should be set back to "needs review" or "waiting for author". Two other items in that list have been around for embarrassingly large amounts of time. "Fix the optimization to skip WAL-logging on table created in same transaction" has been in 10 (!) commitfests. It's seen no substantive action since November. It has a bunch of authors and reviewers listed, Surely somebody can move it forward? "Fix a bug that can prevent standby from restarting" has been around for 6 CFs. This appears to be covered by commit 0668719801838aa6a8bda330ff9b3d20097ea844. Can we just closed this one out? If there's more work left to do another CF item could be added. Of the rest that have been around since January, "pg_rewind corrupts control file global/pg_control" discussion stopped in April. Tom had made some comments to which Michael responded. "Temporary tables prevent autovacuum, leading to XID wraparound" discussion stopped in March, with some possible disagreement between Robert and Tom. "Configuring messages language on Windows" has never been reviewed at all. "Produce a crash dump before main() on Windows", discussion stopped in March. "fix constraint exclusion failure for certain partition key types" appears to need some extra attention, especially from people who have a deep knowledge of constraint exclusion. cheers andrew -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pg_replication_slot_advance to return NULL instead of 0/0 if slot not advanced
On Wed, Jun 06, 2018 at 11:04:39AM +0900, Michael Paquier wrote: > I am attaching as well the patch I sent yesterday. 0001 is candidate > for a back-patch, 0002 is for HEAD to fix the slot advance stuff. I have been able to look again at 0001 and pushed it as 9e149c8. As reading inconsistent data from replication slots is rather hard to trigger, I have just pushed the patch to HEAD. I'll look at 0002 tomorrow. -- Michael signature.asc Description: PGP signature
Re: why partition pruning doesn't work?
On 10 June 2018 at 09:00, Tom Lane wrote: > I wrote: >> One thing I'm wondering about is why in the world are PartitionPruneInfo >> and its subsidiary struct types declared in primnodes.h? That may have been a legacy thing that accidentally wasn't changed from a previous version of the patch. I've now moved it to plannodes.h. > Oh, and while I'm bitching: it seems like there is hardly any part of > the partitioning code in which the comments aren't desperately in need > of a copy-editing pass. They are just chock-full of misspellings, > grammar that is faulty enough to make the meaning unclear, and/or > errors of fact. An example of the latter is the repeated claims that > the basic partitioning functions belong to the planner. Maybe that > was true at some stage of development; but AFAICS the logic in question > now lives in src/backend/partitioning/, which I would not think is > part of the planner. I've made a pass over the execPartition.c and partprune.c code attempting to resolve these issues. I have hopefully fixed them all, but I apologise if I've missed any. I also couldn't resist making a few other improvements to the code. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services run-time_pruning_for_exprs_v5.patch Description: Binary data
Re: PostgreSQL vs SQL Standard
> "Andrew" == Andrew Gierth writes: Andrew> I beat at the grammar a bit to see what it would take to fix it Andrew> at least to the extent of allowing a_expr ColId in a select Andrew> list after removing postfix ops. It looked like it was doable Andrew> by making these keywords more reserved (all of which are Andrew> already reserved words per spec): Andrew> DOUBLE, DAY, FILTER, HOUR, MINUTE, MONTH, OVER, PRECISION, SECOND, Andrew> VARYING, WITHIN, WITHOUT, YEAR oops, also NATIONAL -- Andrew (irc:RhodiumToad)
Re: PostgreSQL vs SQL Standard
> "Tom" == Tom Lane writes: >> I think I got all the issues I currently know of, but there may be >> more, and others may disagree with my classification of issues or the >> rationales for violating the spec. Any feedback? Tom> WRT 1.1 ... I doubt that redefining DROP DOMAIN as you describe Tom> has "no major issues". It sounds to me like an incredibly ugly Tom> wart on the cascaded dependency logic. Quite aside from wartiness, Tom> adding new objects/dependencies as part of a DROP is broken by Tom> design. What if the domain drop has cascaded from something the Tom> domain's constraints themselves depend on? I'd put this as a "has Tom> design-level problems" item. Oh wow, I hadn't noticed that dropping a function referenced from a domain's default or constraint drops the whole domain rather than just removing the default or constraint the way it would with a table. That seems pretty bad to me, in the sense of being potentially a nasty footgun for anyone using domains, but certainly you are correct on the effect on how we categorize the problem. (If it were not the case, then the only way we'd end up cascading to dropping a domain would be if we dropped the base type, in which case the columns are going to go away anyway) Tom> WRT 3.2 on select-list aliases, the postfix-operator issue is only Tom> one of several reasons why we can't support that. There was some Tom> more-detailed discussion about that awhile back, Tom> https://www.postgresql.org/message-id/flat/99ad0450-b1ab-702f-48ef-6972b630bc87%40BlueTreble.com OK, so to summarize, we'd also have to remove ISNULL or make it a reserved word, and also make VARYING reserved (as it is in the spec)? The spec doesn't allow "SELECT col AS reservedword", even though we do, so we don't really have to support "SELECT col reservedword". i.e. we don't need to get all the way to allowing a_expr ColLabel production, it would suffice to get to a_expr ColId. Tom> Constraint name scope: I think it's an overstatement to say that Tom> this makes some info-schema views "useless". "Under-determined" Tom> might be an appropriate word. But in practice that makes it useless except in cases where you generally don't care about i_s anyway. Tom> Or you could say "useless unless the application limits itself to Tom> follow the SQL spec's restriction on names". I'm not sure any applications use i_s to introspect on their own foreign key constraints; every time I've had to give the "that doesn't work because PG's constraint name scope differs from the standard" speech to someone it's because they've been trying to write something more generic than a single application. (Though that could be selection bias I guess.) Someone also pointed out the last time this came up that handling of constraint names on inherited tables means that an application may be unable to avoid using duplicate names. Tom> Object ownership scope: I have not really dug into the spec on Tom> this point, but I recall from our own docs that "schema owner owns Tom> all contained objects too" is true only in DBs that implement some Tom> minimal subset of the standard. The spec literally does not use the term "owns" or "owned by" (in the sense of an authorization identifier owning an object) anywhere except for schemas. If you look at , you'll see that the authorization identifier A which is the recipient of the table's initial GRANTs is defined as being that of the schema. Tom> So that might need more explanation. In any case, we can surely Tom> mount a very strong defense in terms of usability/flexibility Tom> here, we don't need to say it's just historical. Sure. -- Andrew (irc:RhodiumToad)
Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors
Hello Marina,
v9-0003-Pgbench-errors-use-the-ereport-macro-to-report-de.patch
- a patch for the ereport() macro (this is used to report client failures
that do not cause an aborts and this depends on the level of debugging).
ISTM that abort() is called under FATAL.
- implementation: if possible, use the local ErrorData structure during the
errstart()/errmsg()/errfinish() calls. Otherwise use a static variable
protected by a mutex if necessary. To do all of this export the function
appendPQExpBufferVA from libpq.
This patch applies cleanly on top of the other ones (there are minimal
interactions), compiles cleanly, global & pgbench "make check" are ok.
IMO this patch is more controversial than the other ones.
It is not really related to the aim of the patch series, which could do
without, couldn't it? Moreover, it changes pgbench current behavior, which
might be admissible, but should be discussed clearly.
I'd suggest that it should be an independent submission, unrelated to the
pgbench error management patch.
The code adapts/duplicates existing server-side "ereport" stuff and brings
it to the frontend, where the logging needs are somehow quite different.
I'd prefer to avoid duplication and/or have some code sharing. If it
really needs to be duplicated, I'd suggest to put all this stuff in
separated files. If we want to do that, I think that it would belong to
fe_utils, and where it could/should be used by all front-end programs.
I do not understand why names are changed, eg ELEVEL_FATAL instead of
FATAL. ISTM that part of the point of the move would be to be homogeneous,
which suggests that the same names should be reused.
For logging purposes, ISTM that the "elog" macro interface is nicer,
closer to the existing "fprintf(stderr", as it would not introduce the
additional parentheses hack for "rest".
I see no actual value in creating on the fly a dynamic buffer through
plenty macros and functions as the end result is just to print the message
out to stderr in the end.
errfinishImpl: fprintf(stderr, "%s", error->message.data);
This looks like overkill. From reading the code, this does not look
like an improvement:
fprintf(stderr, "invalid socket: %s", PQerrorMessage(st->con));
vs
ereport(ELEVEL_LOG, (errmsg("invalid socket: %s", PQerrorMessage(st->con;
The whole complexity of the server-side interface only make sense because
TRY/CATCH stuff and complex logging requirements (eg several outputs) in
the backend. The patch adds quite some code and complexity without clear
added value that I can see.
The semantics of the existing code is changed, the FATAL levels calls
abort() and replace existing exit(1) calls. Maybe you want an ERROR level
as well.
My 0.02€: maybe you just want to turn
fprintf(stderr, format, ...);
// then possibly exit or abort depending...
into
elog(level, format, ...);
which maybe would exit or abort depending on level, and possibly not
actually report under some levels and/or some conditions. For that, it
could enough to just provide an nice "elog" function.
In conclusion, which you can disagree with because maybe I have missed
something... anyway I currently think that:
- it should be an independent submission
- possibly at "fe_utils" level
- possibly just a nice "elog" function is enough, if so just do that.
--
Fabien.
