On Tue, Feb 2, 2016 at 11:52 PM, Alvaro Herrera <alvhe...@2ndquadrant.com> wrote: > Robert Haas wrote: >> The eventual committer is likely to be much happier with this patch if >> you guys have achieved consensus among yourselves on the best >> approach. > > Actually I imagine that if there's no agreement between author and first > reviewer, there might not *be* a committer in the first place. Perhaps > try to get someone else to think about it and make a decision. It is > possible that some other committer is able to decide by themselves but I > wouldn't count on it.
Pavel and I agree that the backward incompatible behavior is cleaner, but it's backward incompatible. Whether that extra cleanness is worth the incompatibility is never obvious. In this case I think it does. But since my opinion is just my opinion, I was planning to make the committer be that someone else that weighs the issues and makes a decision. I'm new around here and picked this patch to get started due to having Python knowledge and the patch seeming self contained enough. Being new makes me wonder all the time "am I just making life difficult for the patch author or is this idea genuinely good and therefore I should push it forward?". I think more beginners that try to do reviews struggle with this. But, let's try to reach a decision. The existing behaviour dates back to 0bef7ba Add plpython code by Bruce. I've added him to the mail, maybe he can help us with a decision. I'll summarize the patch and explain the incompatibility decision with some illustrative code. The patch is trying to make it possible to call ereport from PL/Python and provide the rich ereport information (detail, hint, sqlcode etc.). There are already functions like plpy.info() but they only expose message and they call elog instead of ereport. See the attached incompat.py. Running it produces: existing behaviour PG elog/ereport with message: 1: hi detail: None hint: None PG elog/ereport with message: ('2: hi', 'another argument') detail: None hint: None PG elog/ereport with message: ('3: hi', 'another argument', 2) detail: None hint: None PG elog/ereport with message: ('4: hi', 'another argument', 2, 'lots', 'of', 'arguments') detail: None hint: None new behaviour PG elog/ereport with message: 1: hi detail: None hint: None PG elog/ereport with message: 2: hi detail: another argument hint: None PG elog/ereport with message: 3: hi detail: another argument hint: 2 Traceback (most recent call last): File "incompat.py", line 43, in <module> info_new('4: hi', 'another argument', 2, 'lots', 'of', 'arguments') TypeError: info_new() takes at most 3 arguments (6 given) I find existing behaviour for 2, 3 and 4 unlike other Python APIs I've seen, surprising and not very useful. If I want to log a tuple I can construct and pass a single tuple, I don't see why plpy.info() needs to construct it for me. And for the documented, single argument call nothing changes. The question to Bruce (and others) is: is it ok to change to the new behaviour illustrated and change meaning for usages like 2, 3 and 4? If we don't want that, the solution Pavel and I see is introducing a parallel API named plpy.raise_info or plpy.rich_info or something like that with the new behaviour and leave the existing functions unchanged. Another option is some compatibility GUC but I don't think it's worth the trouble and confusion.
# simulated PG elog def elog(message): ereport(message) # simulated PG ereport def ereport(message, detail=None, hint=None): print 'PG elog/ereport with message: %s detail: %s hint: %s' % (message, detail, hint) # existing code behaves like this: # takes an unlimited number of arguments # doesn't take keyword arguments # makes a string representation of the tuple of all arguments unless that tuple has size 1 in which case it only makes a string representation of the first one def info_existing(*args): if len(args) == 1: # special case added by Peter in 2e3b16 str_to_log = str(args) else: str_to_log = str(args) elog(str_to_log) # and I'm proposing to change it to do this: # take 1 required argument and extra optional arguments for every argument accepted by ereport # accepts keyword arguments # passing too many arguments will get rejected def info_new(message, detail=None, hint=None): ereport(message, detail, hint) print 'existing behaviour'.center(40) info_existing('1: hi') info_existing('2: hi', 'another argument') info_existing('3: hi', 'another argument', 2) info_existing('4: hi', 'another argument', 2, 'lots', 'of', 'arguments') print print 'new behaviour'.center(40) info_new('1: hi') # for the documented single argument case same behaviour as existing info_new('2: hi', 'another argument') info_new('3: hi', 'another argument', 2) info_new('4: hi', 'another argument', 2, 'lots', 'of', 'arguments')
-- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers