Thread: A wrong comment about search_indexed_tlist_for_var

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)

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

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.

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

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

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