Thread

1. Avoid unused value (src/fe_utils/print.c)

   Ranier Vilela <ranier.vf@gmail.com> — 2023-06-03T10:14:57Z

   Hi,
   
   This is for Postgres 17 (head).
   
   Per Coverity.
   At function print_unaligned_text, variable "need_recordsep", is
   unnecessarily set to true and false.
   
   Attached a trivial fix patch.
   
   regards,
   Ranier Vilela
   

2. Re: Avoid unused value (src/fe_utils/print.c)

   Alexander Lakhin <exclusion@gmail.com> — 2023-06-03T12:00:01Z

   Hello Ranier,
   
   03.06.2023 13:14, Ranier Vilela wrote:
   > Hi,
   >
   > This is for Postgres 17 (head).
   >
   > Per Coverity.
   > At function print_unaligned_text, variable "need_recordsep", is
   > unnecessarily set to true and false.
   
   Clang' scan-build detects 58 errors "Dead assignment", including that one.
   Maybe it would be more sensible to eliminate all errors of this class?
   
   Best regards,
   Alexander
   
   
   
   

3. Re: Avoid unused value (src/fe_utils/print.c)

   Ranier Vilela <ranier.vf@gmail.com> — 2023-06-03T12:46:04Z

   Em sáb., 3 de jun. de 2023 às 09:00, Alexander Lakhin <exclusion@gmail.com>
   escreveu:
   
   > Hello Ranier,
   >
   > 03.06.2023 13:14, Ranier Vilela wrote:
   > > Hi,
   > >
   > > This is for Postgres 17 (head).
   > >
   > > Per Coverity.
   > > At function print_unaligned_text, variable "need_recordsep", is
   > > unnecessarily set to true and false.
   >
   > Clang' scan-build detects 58 errors "Dead assignment", including that one.
   > Maybe it would be more sensible to eliminate all errors of this class?
   >
   Hi Alexander,
   
   Sure.
   I hope that when you or I are a committer,
   we can fix a whole class of bugs together.
   
   best regards,
   Ranier Vilela
   

4. Re: Avoid unused value (src/fe_utils/print.c)

   Michael Paquier <michael@paquier.xyz> — 2023-06-03T22:42:57Z

   On Sat, Jun 03, 2023 at 03:00:01PM +0300, Alexander Lakhin wrote:
   > Clang' scan-build detects 58 errors "Dead assignment", including that one.
   > Maybe it would be more sensible to eliminate all errors of this class?
   
   Depends on if this makes any code changed a bit easier to understand I
   guess, so that would be a case-by-case analysis.  Saying that, the
   proposed patch seems right while it makes slightly easier to
   understand the footer print part.
   --
   Michael
   

5. Re: Avoid unused value (src/fe_utils/print.c)

   Alexander Lakhin <exclusion@gmail.com> — 2023-06-04T04:00:00Z

   Hi Michael,
   
   04.06.2023 01:42, Michael Paquier wrote:
   > On Sat, Jun 03, 2023 at 03:00:01PM +0300, Alexander Lakhin wrote:
   >> Clang' scan-build detects 58 errors "Dead assignment", including that one.
   >> Maybe it would be more sensible to eliminate all errors of this class?
   > Depends on if this makes any code changed a bit easier to understand I
   > guess, so that would be a case-by-case analysis.  Saying that, the
   > proposed patch seems right while it makes slightly easier to
   > understand the footer print part.
   
   It also aligns the code with print_unaligned_vertical(), but I can't see why
   need_recordsep = true;
   is a no-op here (scan-build dislikes only need_recordsep = false;).
   I suspect that removing that line will change the behaviour in cases when
   need_recordsep = false after the loop "print cells" and the loop
   "for (footers)" is executed.
   
   Best regards,
   Alexander
   
   
   
   

6. Re: Avoid unused value (src/fe_utils/print.c)

   Ranier Vilela <ranier.vf@gmail.com> — 2023-06-04T11:59:00Z

   Em dom., 4 de jun. de 2023 às 01:00, Alexander Lakhin <exclusion@gmail.com>
   escreveu:
   
   > Hi Michael,
   >
   > 04.06.2023 01:42, Michael Paquier wrote:
   > > On Sat, Jun 03, 2023 at 03:00:01PM +0300, Alexander Lakhin wrote:
   > >> Clang' scan-build detects 58 errors "Dead assignment", including that
   > one.
   > >> Maybe it would be more sensible to eliminate all errors of this class?
   > > Depends on if this makes any code changed a bit easier to understand I
   > > guess, so that would be a case-by-case analysis.  Saying that, the
   > > proposed patch seems right while it makes slightly easier to
   > > understand the footer print part.
   >
   > It also aligns the code with print_unaligned_vertical(), but I can't see
   > why
   > need_recordsep = true;
   > is a no-op here (scan-build dislikes only need_recordsep = false;).
   > I suspect that removing that line will change the behaviour in cases when
   > need_recordsep = false after the loop "print cells" and the loop
   > "for (footers)" is executed.
   >
   Not sure about this.
   I tested with patch and the results is:
   
   psql -U postgres --no-align
   psql (16beta1)
   WARNING: Console code page (850) differs from Windows code page (1252)
            8-bit characters might not work correctly. See psql reference
            page "Notes for Windows users" for details.
   Type "help" for help.
   
   postgres=# select * from pgbench_accounts limit 100;
   aid|bid|abalance|filler
   1|1|0|
   2|1|0|
   3|1|0|
   4|1|0|
   5|1|0|
   6|1|0|
   7|1|0|
   8|1|0|
   9|1|0|
   etc, etc
   
   psql -U postgres --no-align -P recordsep=";"
   psql (16beta1)
   WARNING: Console code page (850) differs from Windows code page (1252)
            8-bit characters might not work correctly. See psql reference
            page "Notes for Windows users" for details.
   Type "help" for help.
   
   postgres=# select * from pgbench_accounts limit 100;
   aid|bid|abalance|filler;1|1|0|
                                          ;2|1|0|
                                                          ;3|1|0|
   
    ;4|1|0|
                    ;5|1|0|
                                    ;6|1|0|
                                                    ;7|1|0|
                                                                    ;8|1|0|
   
        ;9|1|0|
                        ;10|1|0|
                                        ;11|1|0|
                                                        ;12|1|0|
   
    ;13|1|0|
                    ;14|1|0|
                                    ;15|1|0|
                                                    ;16|1|0|
                                                                    ;17|1|0|
   etc, etc
   
   regards,
   Ranier Vilela
   
   
   >
   > Best regards,
   > Alexander
   >
   

7. Re: Avoid unused value (src/fe_utils/print.c)

   Karina Litskevich <litskevichkarina@gmail.com> — 2023-06-30T14:25:49Z

   Hi,
   
   Alexander wrote:
   
   > It also aligns the code with print_unaligned_vertical(), but I can't see why
   > need_recordsep = true;
   > is a no-op here (scan-build dislikes only need_recordsep = false;).
   > I suspect that removing that line will change the behaviour in cases when
   > need_recordsep = false after the loop "print cells" and the loop
   > "for (footers)" is executed.
   
   As I understand cont->cells is supoused to have all cont->ncolumns * cont->nrows
   entries filled so the loop "print cells" always assigns need_recordsep = true,
   except when there are no cells at all or cancel_pressed == true.
   If cancel_pressed == true then footers are not printed. So to have
   need_recordsep == false before the loop "for (footers)" table should be empty,
   and need_recordsep should be false before the loop "print cells". It can only
   be false there when cont->opt->start_table == true and opt_tuples_only == true
   so that headers are not printed. But when opt_tuples_only == true footers are
   not printed either.
   
   So technically removing "need_recordsep = true;" won't change the outcome. But
   it's not obvious, so I also have doubts about removing this line. If someday
   print options are changed, for example to support printing footers and not
   printing headers, or anything else changes in this function, the output might
   be unexpected with this line removed.
   
   Best regards,
   Karina Litskevich
   Postgres Professional: http://postgrespro.com/
   
   
   
   

8. Re: Avoid unused value (src/fe_utils/print.c)

   Alexander Lakhin <exclusion@gmail.com> — 2023-06-30T16:00:00Z

   Hello Karina,
   
   30.06.2023 17:25, Karina Litskevich wrote:
   > Hi,
   >
   > Alexander wrote:
   >
   >> It also aligns the code with print_unaligned_vertical(), but I can't see why
   >> need_recordsep = true;
   >> is a no-op here (scan-build dislikes only need_recordsep = false;).
   >> I suspect that removing that line will change the behaviour in cases when
   >> need_recordsep = false after the loop "print cells" and the loop
   >> "for (footers)" is executed.
   > As I understand cont->cells is supoused to have all cont->ncolumns * cont->nrows
   > entries filled so the loop "print cells" always assigns need_recordsep = true,
   > except when there are no cells at all or cancel_pressed == true.
   > If cancel_pressed == true then footers are not printed. So to have
   > need_recordsep == false before the loop "for (footers)" table should be empty,
   > and need_recordsep should be false before the loop "print cells". It can only
   > be false there when cont->opt->start_table == true and opt_tuples_only == true
   > so that headers are not printed. But when opt_tuples_only == true footers are
   > not printed either.
   >
   > So technically removing "need_recordsep = true;" won't change the outcome. But
   > it's not obvious, so I also have doubts about removing this line. If someday
   > print options are changed, for example to support printing footers and not
   > printing headers, or anything else changes in this function, the output might
   > be unexpected with this line removed.
   
   I think that the question that should be answered before moving forward
   here is: what this discussion is expected to result in?
   If the goal is to avoid an unused value to make Coverity/clang`s scan-build
   a little happier, then maybe just don't touch other line, that is not
   recognized as dead (at least by scan-build; I wonder what Coverity says
   about that line).
   Otherwise, if the goal is to do review the code and make it cleaner, then
   why not get rid of "if (need_recordsep)" there?
   
   Best regards,
   Alexander
   
   
   
   

9. Re: Avoid unused value (src/fe_utils/print.c)

   Ranier Vilela <ranier.vf@gmail.com> — 2023-06-30T16:15:48Z

   Em sex., 30 de jun. de 2023 às 11:26, Karina Litskevich <
   litskevichkarina@gmail.com> escreveu:
   
   > Hi,
   >
   > Alexander wrote:
   >
   > > It also aligns the code with print_unaligned_vertical(), but I can't see
   > why
   > > need_recordsep = true;
   > > is a no-op here (scan-build dislikes only need_recordsep = false;).
   > > I suspect that removing that line will change the behaviour in cases when
   > > need_recordsep = false after the loop "print cells" and the loop
   > > "for (footers)" is executed.
   >
   > Hi Karina,
   
   
   > As I understand cont->cells is supoused to have all cont->ncolumns *
   > cont->nrows
   > entries filled so the loop "print cells" always assigns need_recordsep =
   > true,
   > except when there are no cells at all or cancel_pressed == true.
   > If cancel_pressed == true then footers are not printed. So to have
   > need_recordsep == false before the loop "for (footers)" table should be
   > empty,
   > and need_recordsep should be false before the loop "print cells". It can
   > only
   > be false there when cont->opt->start_table == true and opt_tuples_only ==
   > true
   > so that headers are not printed. But when opt_tuples_only == true footers
   > are
   > not printed either.
   >
   > So technically removing "need_recordsep = true;" won't change the outcome.
   
   Thanks for the confirmation.
   
   
   > But
   > it's not obvious, so I also have doubts about removing this line. If
   > someday
   > print options are changed, for example to support printing footers and not
   > printing headers, or anything else changes in this function, the output
   > might
   > be unexpected with this line removed.
   
   
   That part I didn't understand.
   How are we going to make this function less readable by removing the
   complicating part.
   
   best regards,
   Ranier Vilela
   

10. Re: Avoid unused value (src/fe_utils/print.c)

    Ranier Vilela <ranier.vf@gmail.com> — 2023-06-30T16:20:10Z

    Em sex., 30 de jun. de 2023 às 13:00, Alexander Lakhin <exclusion@gmail.com>
    escreveu:
    
    > Hello Karina,
    >
    > 30.06.2023 17:25, Karina Litskevich wrote:
    > > Hi,
    > >
    > > Alexander wrote:
    > >
    > >> It also aligns the code with print_unaligned_vertical(), but I can't
    > see why
    > >> need_recordsep = true;
    > >> is a no-op here (scan-build dislikes only need_recordsep = false;).
    > >> I suspect that removing that line will change the behaviour in cases
    > when
    > >> need_recordsep = false after the loop "print cells" and the loop
    > >> "for (footers)" is executed.
    > > As I understand cont->cells is supoused to have all cont->ncolumns *
    > cont->nrows
    > > entries filled so the loop "print cells" always assigns need_recordsep =
    > true,
    > > except when there are no cells at all or cancel_pressed == true.
    > > If cancel_pressed == true then footers are not printed. So to have
    > > need_recordsep == false before the loop "for (footers)" table should be
    > empty,
    > > and need_recordsep should be false before the loop "print cells". It can
    > only
    > > be false there when cont->opt->start_table == true and opt_tuples_only
    > == true
    > > so that headers are not printed. But when opt_tuples_only == true
    > footers are
    > > not printed either.
    > >
    > > So technically removing "need_recordsep = true;" won't change the
    > outcome. But
    > > it's not obvious, so I also have doubts about removing this line. If
    > someday
    > > print options are changed, for example to support printing footers and
    > not
    > > printing headers, or anything else changes in this function, the output
    > might
    > > be unexpected with this line removed.
    >
    > Hi Alexander,
    
    
    > I think that the question that should be answered before moving forward
    > here is: what this discussion is expected to result in?
    >
    I hope to make the function more readable and maintainable.
    
    
    > If the goal is to avoid an unused value to make Coverity/clang`s scan-build
    > a little happier, then maybe just don't touch other line, that is not
    > recognized as dead (at least by scan-build;
    
    
    
    > I wonder what Coverity says
    > about that line).
    >
           if (cont->opt->stop_table)
    477        {
    478                printTableFooter *footers = footers_with_default(cont);
    479
    480                if (!opt_tuples_only && footers != NULL && !
    cancel_pressed)
    481                {
    482                        printTableFooter *f;
    483
    484                        for (f = footers; f; f = f->next)
    485                        {
    486                                if (need_recordsep)
    487                                {
    488                                        print_separator(cont->opt->
    recordSep, fout);
    
    CID 1512766 (#1 of 1): Unused value (UNUSED_VALUE)assigned_value: Assigning
    value false to need_recordsep here, but that stored value is overwritten
    before it can be used.
    489                                        need_recordsep = false;
    490                                }
    491                                fputs(f->data, fout);
    
    value_overwrite: Overwriting previous write to need_recordsep with value
    true.
    492                                need_recordsep = true;
    493                        }
    494                }
    495
    
    
    > Otherwise, if the goal is to do review the code and make it cleaner, then
    > why not get rid of "if (need_recordsep)" there?
    >
    I don't agree with removing this line, as it is essential to print the
    separators, in the footers.
    
    best regards,
    Ranier Vilela
    

11. Re: Avoid unused value (src/fe_utils/print.c)

    Karina Litskevich <litskevichkarina@gmail.com> — 2023-07-06T15:37:16Z

    >
    >
    >
    >> But
    >> it's not obvious, so I also have doubts about removing this line. If
    >> someday
    >> print options are changed, for example to support printing footers and not
    >> printing headers, or anything else changes in this function, the output
    >> might
    >> be unexpected with this line removed.
    >
    >
    > That part I didn't understand.
    > How are we going to make this function less readable by removing the
    > complicating part.
    >
    
    My point is, technically right now you won't see any difference in output
    if you remove the line. Because if we get to that line the need_recordsep
    is already true. However, understanding why it is true is complicated.
    That's
    why if you remove the line people who read the code will wonder why we don't
    need a separator after "fputs"ing a footer. So keeping that line will make
    the code more readable.
    Moreover, removing the line will possibly complicate the future maintenance.
    As I wrote in the part you just quoted, if the function changes in the way
    that need_recordsep is not true right before printing footers any more, then
    output will be unexpected.
    
    Best regards,
    Karina Litskevich
    Postgres Professional: http://postgrespro.com/
    

12. Re: Avoid unused value (src/fe_utils/print.c)

    Marko Tiikkaja <marko@joh.to> — 2023-07-11T22:33:51Z

    On Thu, Jul 6, 2023 at 5:37 PM Karina Litskevich
    <litskevichkarina@gmail.com> wrote:
    > My point is, technically right now you won't see any difference in output
    > if you remove the line. Because if we get to that line the need_recordsep
    > is already true. However, understanding why it is true is complicated. That's
    > why if you remove the line people who read the code will wonder why we don't
    > need a separator after "fputs"ing a footer. So keeping that line will make
    > the code more readable.
    > Moreover, removing the line will possibly complicate the future maintenance.
    > As I wrote in the part you just quoted, if the function changes in the way
    > that need_recordsep is not true right before printing footers any more, then
    > output will be unexpected.
    
    I agree with Karina here.  Either this patch should keep the
    "need_recordsep = true;" line, thus removing the no-op assignment to
    false and making the code slightly less unreadable; or the entire
    function should be refactored for readability.
    
    
    .m
    
    
    
    

13. Re: Avoid unused value (src/fe_utils/print.c)

    Ranier Vilela <ranier.vf@gmail.com> — 2023-07-11T22:46:16Z

    Em ter., 11 de jul. de 2023 às 19:34, Marko Tiikkaja <marko@joh.to>
    escreveu:
    
    > On Thu, Jul 6, 2023 at 5:37 PM Karina Litskevich
    > <litskevichkarina@gmail.com> wrote:
    > > My point is, technically right now you won't see any difference in output
    > > if you remove the line. Because if we get to that line the need_recordsep
    > > is already true. However, understanding why it is true is complicated.
    > That's
    > > why if you remove the line people who read the code will wonder why we
    > don't
    > > need a separator after "fputs"ing a footer. So keeping that line will
    > make
    > > the code more readable.
    > > Moreover, removing the line will possibly complicate the future
    > maintenance.
    > > As I wrote in the part you just quoted, if the function changes in the
    > way
    > > that need_recordsep is not true right before printing footers any more,
    > then
    > > output will be unexpected.
    >
    > I agree with Karina here.  Either this patch should keep the
    > "need_recordsep = true;" line, thus removing the no-op assignment to
    > false and making the code slightly less unreadable; or the entire
    > function should be refactored for readability.
    >
     As there is consensus to keep the no-op assignment,
    I will go ahead and reject the patch.
    
    regards,
    Ranier Vilela
    

14. Re: Avoid unused value (src/fe_utils/print.c)

    Karina Litskevich <litskevichkarina@gmail.com> — 2023-07-21T12:13:02Z

    On Wed, Jul 12, 2023 at 1:46 AM Ranier Vilela <ranier.vf@gmail.com> wrote:
    
    >  As there is consensus to keep the no-op assignment,
    > I will go ahead and reject the patch.
    >
    
    In your patch you suggest removing two assignments, and we only have
    consensus to keep one of them. The other one is an obvious no-op.
    
    I attached a patch that removes only one assignment. Could you please try
    it and check whether Coverity is still complaining about need_recordsep
    variable?
    
    Best regards,
    Karina Litskevich
    Postgres Professional: http://postgrespro.com/
    

15. Re: Avoid unused value (src/fe_utils/print.c)

    Ranier Vilela <ranier.vf@gmail.com> — 2023-07-24T22:04:27Z

    Em sex., 21 de jul. de 2023 às 09:13, Karina Litskevich <
    litskevichkarina@gmail.com> escreveu:
    
    >
    >
    > On Wed, Jul 12, 2023 at 1:46 AM Ranier Vilela <ranier.vf@gmail.com> wrote:
    >
    >>  As there is consensus to keep the no-op assignment,
    >> I will go ahead and reject the patch.
    >>
    >
    > In your patch you suggest removing two assignments, and we only have
    > consensus to keep one of them. The other one is an obvious no-op.
    >
    > I attached a patch that removes only one assignment. Could you please try
    > it and check whether Coverity is still complaining about need_recordsep
    > variable?
    >
    Yeah.
    Checked today, Coverity does not complain about need_recordsep.
    
    best regards,
    Ranier Vilela
    

16. Re: Avoid unused value (src/fe_utils/print.c)

    Karina Litskevich <litskevichkarina@gmail.com> — 2023-07-28T08:53:09Z

    On Tue, Jul 25, 2023 at 1:04 AM Ranier Vilela <ranier.vf@gmail.com> wrote:
    
    > Checked today, Coverity does not complain about need_recordsep.
    >
    
    Great! Thanks.
    So v2 patch makes Coverity happy, and as for me doesn't make the code
    less readable. Does anyone have any objections?
    
    Best regards,
    Karina Litskevich
    Postgres Professional: http://postgrespro.com/