Thread

Re: [Patch] Windows relation extension failure at 2GB and 4GB

Bryan Green <dbryan.green@gmail.com> — 2025-11-27T07:52:33Z

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