Horton

Thread

Commits

  1. Fix comment about cross-checking the varnullingrels

  2. Convert nullingrels match checks from Asserts to test-and-elog.

  1. A wrong comment about search_indexed_tlist_for_var

    Richard Guo <guofenglinux@gmail.com> — 2023-12-01T11:12:34Z

    The comment of search_indexed_tlist_for_var says:

 * In debugging builds, we cross-check the varnullingrels of the subplan
 * output Var based on nrm_match.

However, this cross-check will also be performed in non-debug builds
ever since commit 867be9c07, which converts this check from Asserts to
test-and-elog.  The commit message there also says:

    Committed separately with the idea that eventually we'll revert
    this.  It might be awhile though.

I wonder if now is the time to revert it, since there have been no
related bugs reported for quite a while.  Otherwise I think we may need
to revise the comment of search_indexed_tlist_for_var to clarify that
the cross-check is not limited to debugging builds.

Please note that if we intend to revert commit 867be9c07, we need to
revert 69c430626 too.

Thanks
Richard

  2. Re: A wrong comment about search_indexed_tlist_for_var

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-12-01T12:25:31Z

    On 2023-Dec-01, Richard Guo wrote:

> However, this cross-check will also be performed in non-debug builds
> ever since commit 867be9c07, which converts this check from Asserts to
> test-and-elog.  The commit message there also says:
> 
>     Committed separately with the idea that eventually we'll revert
>     this.  It might be awhile though.
> 
> I wonder if now is the time to revert it, since there have been no
> related bugs reported for quite a while.

I don't know anything about this, but maybe it would be better to let
these elogs there for longer, so that users have time to upgrade and
test.  This new code has proven quite tricky, and if I understand
correctly, if we do run some query with wrong varnullingrels in
production code without elog and where Assert() does nothing, that might
silently lead to wrong results.

OTOH keeping the elog there might impact performance.  Would that be
significant?

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Debido a que la velocidad de la luz es mucho mayor que la del sonido,
 algunas personas nos parecen brillantes un minuto antes
 de escuchar las pelotudeces que dicen." (Roberto Fontanarrosa)

  3. Re: A wrong comment about search_indexed_tlist_for_var

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-12-01T18:27:08Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2023-Dec-01, Richard Guo wrote:
>> However, this cross-check will also be performed in non-debug builds
>> ever since commit 867be9c07, which converts this check from Asserts to
>> test-and-elog.  The commit message there also says:
>> Committed separately with the idea that eventually we'll revert
>> this.  It might be awhile though.
>> I wonder if now is the time to revert it, since there have been no
>> related bugs reported for quite a while.

> I don't know anything about this, but maybe it would be better to let
> these elogs there for longer, so that users have time to upgrade and
> test.

Yeah.  It's good that we've not had field reports against 16.0 or 16.1,
but we can't really expect that 16.x has seen widespread adoption yet.
I do think we should revert this eventually, but I'd wait perhaps
another year.

> OTOH keeping the elog there might impact performance.  Would that be
> significant?

Doubt it'd be anything measurable, in comparison to all the other
stuff the planner does.

			regards, tom lane

  4. Re: A wrong comment about search_indexed_tlist_for_var

    Richard Guo <guofenglinux@gmail.com> — 2023-12-04T08:42:02Z

    On Sat, Dec 2, 2023 at 2:27 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > On 2023-Dec-01, Richard Guo wrote:
> >> However, this cross-check will also be performed in non-debug builds
> >> ever since commit 867be9c07, which converts this check from Asserts to
> >> test-and-elog.  The commit message there also says:
> >> Committed separately with the idea that eventually we'll revert
> >> this.  It might be awhile though.
> >> I wonder if now is the time to revert it, since there have been no
> >> related bugs reported for quite a while.
>
> > I don't know anything about this, but maybe it would be better to let
> > these elogs there for longer, so that users have time to upgrade and
> > test.
>
> Yeah.  It's good that we've not had field reports against 16.0 or 16.1,
> but we can't really expect that 16.x has seen widespread adoption yet.
> I do think we should revert this eventually, but I'd wait perhaps
> another year.


Then here is a trivial patch to adjust the comment, which should get
reverted along with 867be9c07.

Thanks
Richard

  5. Re: A wrong comment about search_indexed_tlist_for_var

    Matt Skelley <mattskelley.au@gmail.com> — 2023-12-18T15:26:40Z

    The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            tested, passed

Comment is updated correctly.

  6. Re: A wrong comment about search_indexed_tlist_for_var

    Robert Haas <robertmhaas@gmail.com> — 2024-05-14T17:07:40Z

    On Mon, Dec 4, 2023 at 3:42 AM Richard Guo <guofenglinux@gmail.com> wrote:
> Then here is a trivial patch to adjust the comment, which should get
> reverted along with 867be9c07.

Richard, since you're a committer now, maybe you'd like to commit
this. I don't really understand the portion of your commit message
inside the parentheses and would suggest that you just delete that,
but the rest seems fine.

If you do commit it, also update the status at
https://commitfest.postgresql.org/48/4683/

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  7. Re: A wrong comment about search_indexed_tlist_for_var

    Richard Guo <guofenglinux@gmail.com> — 2024-05-16T09:58:32Z

    On Wed, May 15, 2024 at 1:07 AM Robert Haas <robertmhaas@gmail.com> wrote:

> On Mon, Dec 4, 2023 at 3:42 AM Richard Guo <guofenglinux@gmail.com> wrote:
> > Then here is a trivial patch to adjust the comment, which should get
> > reverted along with 867be9c07.
>
> Richard, since you're a committer now, maybe you'd like to commit
> this. I don't really understand the portion of your commit message
> inside the parentheses and would suggest that you just delete that,
> but the rest seems fine.
>
> If you do commit it, also update the status at
> https://commitfest.postgresql.org/48/4683/


Thank you for the suggestion.  Yeah, this is a good candidate for my
first commit. :-)  I will aim to do it during the next commitfest.

Thanks
Richard

  8. Re: A wrong comment about search_indexed_tlist_for_var

    Robert Haas <robertmhaas@gmail.com> — 2024-05-16T12:42:33Z

    On Thu, May 16, 2024 at 5:58 AM Richard Guo <guofenglinux@gmail.com> wrote:
> Thank you for the suggestion.  Yeah, this is a good candidate for my
> first commit. :-)  I will aim to do it during the next commitfest.

You don't need to wait for the next CommitFest to fix a comment (or a
bug). And, indeed, it's better if you do this before we branch.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  9. Re: A wrong comment about search_indexed_tlist_for_var

    Richard Guo <guofenglinux@gmail.com> — 2024-06-10T07:04:10Z

    On Thu, May 16, 2024 at 8:42 PM Robert Haas <robertmhaas@gmail.com> wrote:
> You don't need to wait for the next CommitFest to fix a comment (or a
> bug). And, indeed, it's better if you do this before we branch.

Patch pushed and the CF entry closed.  Thank you for the suggestion.

Thanks
Richard