On Thu, 27 Mar 2025 at 14:51, Michael Paquier <mich...@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
