On Thu, 27 Mar 2025 at 14:51, Michael Paquier <michael@paquier.xyz> wrote:
> One habit that I've found really useful to do when it comes to this
> area of the code is to apply the tests first to show what kind of
> behavior we had before changing the jumbling, then apply the update to
> the query jumbling. This has two benefits:
> - If the update of the second phase gets reverted, we still have the
> tests.
> - It is possible to show in the git history how the behavior changes,
> analyzing them in isolation.
>
> So I'd suggest an extra split, with the tests committed first.
I didn't do that. I can't quite process the logic of adding tests to
check we get the wrong behaviour.
> Maybe hide that in the header with one extra USE_ASSERT_CHECKING if we
> are only going to increment it when assertions are enabled?
>
> + * These are flushed out to the jumble buffer before subsequent appends
> + * and before performing the final jumble hash.
I thought about that and had decided to leave it in. I had been
concerned about the struct growing and shrinking between
cassert/non-cassert builds. It's probably ok since it's expected to be
an internal field. I guess if a cassert extension used the field and
ran against a non-cassert PostgreSQL, then they'd have trouble. I did
end up removing it in what I just pushed. I felt I might be being
overly concerned since the field is at the end of the struct and is
commented that it's for Asserts only.
> This comment is slightly incorrect when 0001 is taken in isolation?
> The flush of the NULL counter is done in the patch via
> FlushPendingNulls() when jumbling the final value, not before more
> appends? It becomes true with 0002, where FlushPendingNulls() is
> called before appending any data.
That was my mistake. I divided the patches incorrectly. There was
meant to be a flush in there.
> Perhaps removing JUMBLE_FIELD_SINGLE() is better now.
Done and pushed.
Thanks.
David
