On Fri, Apr 22, 2016 at 5:26 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Apr 22, 2016 at 1:07 PM, Andres Freund <andres@anarazel.de> wrote:
>> The attached patch basically adds the segment size checks to
>> _mdfd_getseg(), and doesn't perform extension, even in recovery, if
>> EXTENSION_REALLY_RETURN_NULL is passed.
>>
>> This fixes the issue for me, both in the originally reported variant,
>> and in some more rudely setup version (i.e. rm'ing files).
>
> I think this looks broadly reasonable. Some specific comments:
>
> + /*
> + * Normally we will create new segments only if authorized by
> + * the caller (i.e., we are doing mdextend()). But when doing
> + * WAL recovery, create segments anyway; this allows cases
> + * such as replaying WAL data that has a write into a
> + * high-numbered segment of a relation that was later deleted.
> + * We want to go ahead and create the segments so we can
> + * finish out the replay.
>
> Add something like: "However, if the caller has specified
> EXTENSION_REALLY_RETURN_NULL, then extension is not desired even in
> recovery; we won't reach this point in that case."
>
> + errno = ENOENT; /* some callers check errno, yuck */
>
> I think a considerably more detailed comment would be warranted.
>
> + else
> + {
> + ereport(ERROR,
> + (errcode_for_file_access(),
> + errmsg("could not open file \"%s\"
> (target block %u): previous segment is only %u blocks",
> + _mdfd_segpath(reln, forknum, nextsegno),
> + blkno, nblocks)));
> + }
>
> The else and its ensuing level of indentation are unnecessary.
Andres, this issue has now been open for more than a month, which is
frankly kind of ridiculous given the schedule we're trying to hit for
beta. Do you think it's possible to commit something RSN without
compromising the quality of PostgreSQL 9.6?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company