Re: Review: Patch to compute Max LSN of Data Pages

>I think we should expect provided path to be relative to current directory
> or may consider it to be relative to either one of Data or CWD.
>Because normally we expect path to be relative to CWD if some program is
> asking for path in command line.

Please find the attached patch to make the path relative to CWD and check if the path is under data directory.

Now the only point raised by Alvaro and you for this patch which is not yet addressed is :

> Hmm. I think I'd expect that if I give pg_computemaxlsn a number, it
> should consider that it is a relfilenode, and so it should get a list of
> all segments for all forks of it. So if I give "12345" it should get
> 12345, 12345.1 and so on, and also 12345_vm 12345_vm.1 and so on.
> However, if what I give it is a path, i.e. it contains a slash, I think
> it should only consider the specific file mentioned. In that light, I'm
> not sure that command line options chosen are the best set.
I am just not sure whether we should handle this functionality and if we have to handle what is better way to provide it to user.
Shall we provide new option -r or something for it.

Opinions/Suggestions?

IMHO, such functionality can be easily extendable in future.
However I have no problem in implementing such functionality if you are of opinion that this is basic and it should go with first version of feature.

With Regards,
Amit Kapila.

Hi Amit

On Mon, Dec 3, 2012 at 6:56 PM, Amit kapila <amit(dot)kapila(at)huawei(dot)com> wrote:

> >I think we should expect provided path to be relative to current
> directory
> > or may consider it to be relative to either one of Data or CWD.
> >Because normally we expect path to be relative to CWD if some program is
> > asking for path in command line.
>
> Please find the attached patch to make the path relative to CWD and check
> if the path is under data directory.
>

Works good now. Although I am thinking why are you disallowing the absolute
path of file. Any particular reason?

>
> Now the only point raised by Alvaro and you for this patch which is not
> yet addressed is :
>
> > Hmm. I think I'd expect that if I give pg_computemaxlsn a number, it
> > should consider that it is a relfilenode, and so it should get a list of
> > all segments for all forks of it. So if I give "12345" it should get
> > 12345, 12345.1 and so on, and also 12345_vm 12345_vm.1 and so on.
> > However, if what I give it is a path, i.e. it contains a slash, I think
> > it should only consider the specific file mentioned. In that light, I'm
> > not sure that command line options chosen are the best set.
> I am just not sure whether we should handle this functionality and if we
> have to handle what is better way to provide it to user.
> Shall we provide new option -r or something for it.
>
> Opinions/Suggestions?
>
> IMHO, such functionality can be easily extendable in future.
> However I have no problem in implementing such functionality if you are of
> opinion that this is basic and it should go with first version of feature.
>
>

I also had a similar point made by Alvaro to allow all the segments of the
relation for a given relation file name, or add another option do do the
same. But if everybody is fine with leaving it for the future, I do not
have any further concerns with the patch. It is good from my side.

With Regards,
> Amit Kapila.
>
>

Thanks
Muhammad Usama

Hi Muhammad,

On Friday, December 07, 2012 7:43 PM Muhammad Usama wrote:
>Hi Amit

On Mon, Dec 3, 2012 at 6:56 PM, Amit kapila <amit(dot)kapila(at)huawei(dot)com<mailto:amit(dot)kapila(at)huawei(dot)com>> wrote:
>>I think we should expect provided path to be relative to current directory
>> or may consider it to be relative to either one of Data or CWD.
>>Because normally we expect path to be relative to CWD if some program is
>> asking for path in command line.

> Please find the attached patch to make the path relative to CWD and check if the path is under data directory.

> Works good now.

Thank you for verification.

> Although I am thinking why are you disallowing the absolute path of file. Any particular reason?

The reason to disallow absolute path is that, we need to test on multiple platforms and to keep the scope little less.
I thought we can allow absolute paths in future.

> I also had a similar point made by Alvaro to allow all the segments of the relation for a given relation file name, or add another option do do the same. But if everybody is fine with leaving it for the future, I do > not have any further concerns with the patch. It is good from my side.

In my opinion we can extend the utility in future for both the below points suggested:
1. allow absolute paths in file path
2. allow to get max lsn for relation segments.

If you are also okay, then we can proceed and let Committer also share his opinion.

Thank you for reviewing the patch.

With Regards,
Amit Kapila.

Amit kapila <amit(dot)kapila(at)huawei(dot)com> writes:
> On Friday, December 07, 2012 7:43 PM Muhammad Usama wrote:
>> Although I am thinking why are you disallowing the absolute path of file. Any particular reason?

> The reason to disallow absolute path is that, we need to test on multiple platforms and to keep the scope little less.

This argument seems to me to be completely nuts. What's wrong with an
absolute path? Wouldn't you have to go out of your way to disallow it?

regards, tom lane

On Saturday, December 08, 2012 9:44 AM Tom Lane wrote:
Amit kapila <amit(dot)kapila(at)huawei(dot)com> writes:
> On Friday, December 07, 2012 7:43 PM Muhammad Usama wrote:
>>> Although I am thinking why are you disallowing the absolute path of file. Any particular reason?

>> The reason to disallow absolute path is that, we need to test on multiple platforms and to keep the scope little less.

>This argument seems to me to be completely nuts. What's wrong with an
>absolute path? Wouldn't you have to go out of your way to disallow it?

There's nothing wrong with absolute path. I have updated the patch to work for absolute path as well.
Updated patch attached with this mail.

With Regards,
Amit Kapila.