Review: query result history in psql

From: ian link <ian(at)ilink(dot)io>
To: PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Review: query result history in psql
Date: 2013-06-25 21:29:15
Message-ID: CAOOwM5+bhSnxTyEs=k_UdR2xROcfRPOxHQOfqP60+TT_pahwcA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi Maciej,

I've been reviewing your patch for the ongoing commitfest. First let me say
that I think it's a great idea and provides some very useful functionality.

However, there are a few minor problems. There were a few
english/grammatical mistakes that I went ahead and fixed. Additionally, I
think some of the string manipulation might be placed outside of the main
ans.c file. I don't know if there's a better place for 'EscapeForCopy' and
'GetEscapedLen'. Not really a big deal, just an organizational idea. I also
changed 'EscapeForCopy' to 'EscapeAndCopy'. I think that better describes
the functionality. 'EscapeForCopy' kind of implies that another function is
needed to copy the string.

What does 'ans' stand for? I am not sure how it relates to the concept of a
query history. It didn't stop my understanding of the code, but I don't
know if a user will immediately know the meaning.

Probably the biggest problem is that the query history list is missing a
maximum size variable. I think this could be valuable for preventing users
from shooting themselves in the foot. If the user is running large queries,
they might accidentally store too much data. This probably somewhat of an
edge-case but I believe it is worth considering. We could provide a
sensible default limit (10 queries?) and also allow the user to change it.

Finally, is it worth resetting the query history every time a user
reconnects to the database? I can see how this might interrupt a user's
workflow. If the user suddenly disconnects (network connection interrupted,
etc) then they would lose their history. I think this is definitely up for
debate. It would add more management overhead (psql options etc) and might
just be unnecessary. However, with a sane limit to the size of the query
history, I don't know if there would be too many drawbacks from a storage
perspective.

Those issues aside - I think it's a great feature! I can add the
grammatical fixes I made whenever the final patch is ready. Or earlier,
whatever works for you. Also, this is my first time reviewing a patch, so
please let me know if I can improve on anything. Thanks!

Ian Link

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Antonin Houska 2013-06-25 21:50:02 Re: LATERAL quals revisited
Previous Message Jeff Janes 2013-06-25 21:18:01 Re: XLogInsert scaling, revisited