Hello

This patch remove redundant rows from PL/pgSQL executor (-89 lines).
Doesn't change a functionality.

Regards

Pavel Stehule

Search Discussions

  • Tom Lane at Dec 17, 2010 at 3:14 pm

    Pavel Stehule writes:
    This patch remove redundant rows from PL/pgSQL executor (-89 lines).
    Doesn't change a functionality.
    I'm not really impressed with this idea: there's no a priori reason
    that all those loop types would necessarily have exactly the same
    control logic.

    regards, tom lane
  • Pavel Stehule at Dec 17, 2010 at 3:25 pm

    2010/12/17 Tom Lane <tgl@sss.pgh.pa.us>:
    Pavel Stehule <pavel.stehule@gmail.com> writes:
    This patch remove redundant rows from PL/pgSQL executor (-89 lines).
    Doesn't change a functionality.
    I'm not really impressed with this idea: there's no a priori reason
    that all those loop types would necessarily have exactly the same
    control logic.
    this code processes a rc from EXIT, CONTINUE and RETURN statement. All
    these statements are defined independent to outer loops, so there are
    no reason why this code has be different. And actually removed code
    was almost same. There was different a process for FOR statement,
    because there isn't possible direct "return" from cycle, because is
    necessary to release a allocated memory.

    There is no reason why the processing should be same, but actually is same.

    regards, tom lane
  • Tom Lane at Dec 17, 2010 at 3:36 pm

    Pavel Stehule writes:
    2010/12/17 Tom Lane <tgl@sss.pgh.pa.us>:
    I'm not really impressed with this idea: there's no a priori reason
    that all those loop types would necessarily have exactly the same
    control logic.
    There is no reason why the processing should be same, but actually is same.
    Yes, and it might need to be different in future, whereupon this patch
    would make life extremely difficult.

    regards, tom lane
  • Pavel Stehule at Dec 17, 2010 at 3:42 pm

    2010/12/17 Tom Lane <tgl@sss.pgh.pa.us>:
    Pavel Stehule <pavel.stehule@gmail.com> writes:
    2010/12/17 Tom Lane <tgl@sss.pgh.pa.us>:
    I'm not really impressed with this idea: there's no a priori reason
    that all those loop types would necessarily have exactly the same
    control logic.
    There is no reason why the processing should be same, but actually is same.
    Yes, and it might need to be different in future, whereupon this patch
    would make life extremely difficult.
    Do you know about some possible change?

    I can't to argument with this argument. But I can use a same argument.
    Isn't be more practical to have a centralized management for return
    code? There is same probability to be some features in future that
    will need a modify this fragment - for example a new return code
    value. Without centralized management, you will have to modify four
    fragments instead one.

    Regards

    Pavel Stehule
    regards, tom lane
  • Alvaro Herrera at Dec 17, 2010 at 3:18 pm

    Excerpts from Pavel Stehule's message of vie dic 17 07:02:00 -0300 2010:
    Hello

    This patch remove redundant rows from PL/pgSQL executor (-89 lines).
    Doesn't change a functionality.
    Hmm I'm not sure but I think the new code has some of the result values
    inverted. Did you test this thoroughly? I think this would be a nice
    simplification because the repetitive coding is ugly and confusing, but
    I'm nervous about the unstated assumption that all loop structs are
    castable to the new struct. Seems like it could be easily broken in the
    future.

    --
    Álvaro Herrera <alvherre@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
  • Pavel Stehule at Dec 17, 2010 at 3:32 pm

    2010/12/17 Alvaro Herrera <alvherre@commandprompt.com>:
    Excerpts from Pavel Stehule's message of vie dic 17 07:02:00 -0300 2010:
    Hello

    This patch remove redundant rows from PL/pgSQL executor (-89 lines).
    Doesn't change a functionality.
    Hmm I'm not sure but I think the new code has some of the result values
    inverted.  Did you test this thoroughly?  I think this would be a nice
    simplification because the repetitive coding is ugly and confusing, but
    I'm nervous about the unstated assumption that all loop structs are
    castable to the new struct.  Seems like it could be easily broken in the
    future.
    All regress tests was successful.

    A common structure isn't a new. There is same for FOR loops, there is
    some similar in parser yylval, and it is only explicit description of
    used construction for stmt structures. I should not to modify any
    other structure. But I am not strong in this. A interface can be
    changed and enhanced about pointer to label. Just I am not satisfied
    from current state, where same things are implemented with minimal
    difference.

    Pavel

    --
    Álvaro Herrera <alvherre@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
  • Stephen Frost at Jan 19, 2011 at 7:31 pm
    Greetings,

    * Pavel Stehule (pavel.stehule@gmail.com) wrote:
    This patch remove redundant rows from PL/pgSQL executor (-89 lines).
    While I can certainly appreciate wanting to remove redundant code, I
    don't think this change actually improves the situation. The problem is
    more than just that we might want to make a change to 'while' but not
    'for', it's also that it makes it very difficult to follow the code
    flow.

    If you could have found a way to make it work for all calls to
    exec_stmts(), it might be a bit better, but you can't without kludging
    it up further. If you could, then it might be possible to move some of
    this logic *into* exec_stmts(), but I don't see that being reasonable
    either.

    In the end, I'd recommend cleaning up the handling of the exec_stmts()
    return code so that all of these pieces follow the same style and look
    similar (I'd go with the switch-based approach and remove the if/else
    branches). That'll make it easier for anyone coming along later who
    does end up needing to change all three.
    Doesn't change a functionality.
    I'm not convinced of this either, to be honest.. In exec_stmt_while(),
    there is:

    for(;;)
    {
    [...]
    if (isnull || !value)
    break;

    rc = exec_stmts(estate, stmt->body);
    [...]
    }
    return PLPGSQL_RC_OK;

    You replaced the last return with:

    return rc;

    Which could mean returning an uninitialized rc if the above break;
    happens.

    In the end, I guess it's up to the committers on how they feel about
    this, so here's an updated patch which fixes the above issue and
    improves the comments/grammer. Passes all regressions and works in my
    limited testing.

    commit e6639d83db5b301e184bf158b1591007a3f79526
    Author: Stephen Frost <sfrost@snowman.net>
    Date: Wed Jan 19 14:28:36 2011 -0500

    Improve comments in pl_exec.c wrt can_leave_loop()

    This patch improves some of the comments around can_leave_loop(), and
    fixes a couple of fall-through cases to make sure they behave the same
    as before the code de-duplication patch that introduced
    can_leave_loop().

    commit f8262adcba662164dbc24d289cb036b3f0aee582
    Author: Stephen Frost <sfrost@snowman.net>
    Date: Wed Jan 19 14:26:27 2011 -0500

    remove redundant code from pl_exec.c

    This patch removes redundant handling of exec_stmts()'s return code
    by creating a new function to be called after exec_stmts() is run.
    This new function will then handle the return code, update it if
    necessary, and return if the loop should continue or not.

    Patch by Pavel Stehule.

    Thanks,

    Stephen
  • Tom Lane at Jan 19, 2011 at 8:34 pm

    Stephen Frost writes:
    While I can certainly appreciate wanting to remove redundant code, I
    don't think this change actually improves the situation. The problem is
    more than just that we might want to make a change to 'while' but not
    'for', it's also that it makes it very difficult to follow the code
    flow.
    That was my reaction as well; and I was also concerned that this could
    have a non-negligible performance price. (At the very least it's adding
    an additional function call per loop execution, and there could also be
    a penalty from forcing "rc" to be in memory rather than a register.)

    I think we should reject this one.
    In the end, I'd recommend cleaning up the handling of the exec_stmts()
    return code so that all of these pieces follow the same style and look
    similar (I'd go with the switch-based approach and remove the if/else
    branches). That'll make it easier for anyone coming along later who
    does end up needing to change all three.
    Using a switch there is a bit problematic since in some cases you want
    to use "break" to exit the loop. We could replace such breaks by gotos,
    but that would be another strike against the argument that you're making
    things more readable. I think the switch in exec_stmt_loop is only
    workable because it has no cleanup to do, so it can just "return" in
    places where a loop break would otherwise be needed. In short: if you
    want to make these all look alike, better to go the other way.

    regards, tom lane
  • Stephen Frost at Jan 19, 2011 at 8:36 pm

    * Tom Lane (tgl@sss.pgh.pa.us) wrote:
    I think we should reject this one.
    Works for me.
    Using a switch there is a bit problematic since in some cases you want
    to use "break" to exit the loop. We could replace such breaks by gotos,
    but that would be another strike against the argument that you're making
    things more readable. I think the switch in exec_stmt_loop is only
    workable because it has no cleanup to do, so it can just "return" in
    places where a loop break would otherwise be needed. In short: if you
    want to make these all look alike, better to go the other way.
    Ah, yeah, good point. We do use gotos elsewhere for this reason, might
    consider revisiting those also, if we're trying to a 'clean-up' patch.
    In any case, I'll mark this as rejected.

    Thanks!

    Stephen
  • Pavel Stehule at Jan 20, 2011 at 7:56 am

    2011/1/19 Stephen Frost <sfrost@snowman.net>:
    * Tom Lane (tgl@sss.pgh.pa.us) wrote:
    I think we should reject this one.
    Works for me.
    Using a switch there is a bit problematic since in some cases you want
    to use "break" to exit the loop.  We could replace such breaks by gotos,
    but that would be another strike against the argument that you're making
    things more readable.  I think the switch in exec_stmt_loop is only
    workable because it has no cleanup to do, so it can just "return" in
    places where a loop break would otherwise be needed.  In short: if you
    want to make these all look alike, better to go the other way.
    Ah, yeah, good point.  We do use gotos elsewhere for this reason, might
    consider revisiting those also, if we're trying to a 'clean-up' patch.
    In any case, I'll mark this as rejected.
    ok, I don't thinking so current code is readable, but I can't to do better now.

    Thank you for review.

    Regards

    Pavel Stehule
    Thanks!

    Stephen

    -----BEGIN PGP SIGNATURE-----
    Version: GnuPG v1.4.10 (GNU/Linux)

    iEYEARECAAYFAk03S10ACgkQrzgMPqB3kigWdQCeIU/dvgJ8bMVZ7nh+TYiFHVlP
    4H4AnR/JbboMWbCu95G2aUEcP3LZDDGM
    =R8c6
    -----END PGP SIGNATURE-----

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedDec 17, '10 at 10:02a
activeJan 20, '11 at 7:56a
posts11
users4
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2022 Grokbase