On 11/14/2025 12:44 AM, Michael Paquier wrote:
> On Thu, Nov 13, 2025 at 10:58:54AM -0600, Bryan Green wrote:
>> On 11/12/2025 10:05 PM, Michael Paquier wrote:
>>>> Moving on to the I/O routine changes. There was a little bit of
>>> noise in the diffs, like one more comment removed that should still be
>>> around. Indentation has needed some adjustment as well, there was one
>>> funny diff with a cast to pgoff_t. And done this part as a first
>>> step, because that's already a nice cut.
>>
>> Apologies for the state of this and your loss of time. I was testing a
>> new workflow and attached the wrong revision. No excuse, just what
>> happened. I will be more careful and do a closer review of diffs going
>> forward.
>
> No worries. Thanks for all your efforts here.
>
>> I had started down the path of using sql and doing regression testing
>> and decide instead that a tap test would be better for my specific case
>> of testing on Windows.
>
> How much do we really care about the case of FSCTL_SET_SPARSE? We
> don't use it in the tree, and I doubt we will, but perhaps you have
> some plans to use it for something I am unaware of, that would justify
> its existence?
>
No plans for it. Dropped.
>> The argument for a TAP test in this case would be File::Temp handles
>> cleanup automatically for us (even on test failure). Also, no need for
>> alternate output files.
>>
>> I agree we should go to a cross-platform test. I'm 51/49 in favor of
>> using TAP tests still, but if you, or others, feel strongly otherwise, I
>> can restructure it to work that way.
>
> There are a couple of options here:
> - Use NO_INSTALLCHECK so as the test would never be run on an existing
> deployment, only check. We could use that on top of a PG_TEST_EXTRA
> to check with a large offset if the writes cannot be cheap..
> - For alternate output, the module could have a SQL function that
> returns the size of off_t or equivalent, mixed with an \if to avoid
> the test for a sizeof 4 bytes.
>
> If others argue in favor of a TAP test as well, that's OK by me.
> However, there is nothing in the current patch that justifies that:
Agreed. I've reworked this as a SQL regression test per your suggestions.
The test now uses OpenTemporaryFile() via the VFD layer, which handles
cleanup automatically, so there's no need for TAP's File::Temp. A
test_large_files_offset_size() function returns sizeof(pgoff_t), and
the SQL uses \if to skip on platforms where that's less than 8 bytes.
NO_INSTALLCHECK is set.
One issue came up during testing: at 2GB+1, the OVERLAPPED.OffsetHigh
field is naturally zero, so commenting out the OffsetHigh fix didn't
cause the test to fail. I've changed the test offset to 4GB+1 where
OffsetHigh must be non-zero. The test now catches both bugs. FileSize()
provides independent verification that writes actually reached the
correct offset.
I have changed the name of the patch to reflect that it is not just
adding tests, but includes the change for the problem.
Updated patch attached.
> --
> Michael
--
Bryan Green
EDB: https://www.enterprisedb.com
